Make sure to use the default decimal context in our code #4231
Conversation
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| # to a float (True becomes 1.0 and False becomes 0.0) | ||
| self.sampled = self._sample_rand < self.sample_rate | ||
| # Now we roll the dice. | ||
| self.sampled = self._sample_rand < Decimal.from_float(self.sample_rate) |
There was a problem hiding this comment.
Why do we need this? Just wondering if this can also potentially fail due to unforeseen circumstances 🙃
There was a problem hiding this comment.
when people set the FloatOperation trap (like the user in the linked issue did) then the old comparison fails, because then comparing Decimals to floats is not allowed anymore. I guess we also could add this around the comparison:
with localcontext(DefaultContext) as ctx:
ctx.prec = 6
self.sampled = self._sample_rand < self.sample_rate
There was a problem hiding this comment.
I don't really have a preference, but maybe wrapping stuff in a localcontext might be good, just to possibly avoid other unforeseen errors? I'm kinda afraid of the whole decimal API now so I'd make this as defensive as possible.
There was a problem hiding this comment.
I added a small suggestion inline, but wanted to second what @sentrivana said about this whole Decimal API making me uneasy.
When I implemented sample_rand using Decimal, I thought it would make our lives easier by making it simple to ensure the correct precision for sample_rand.
However, now that multiple issues have come up related to using Decimal, I think it might have added more complexity than I realized, and perhaps we should consider revisiting the decision to use Decimal altogether (especially if another related issue comes up)
| Decimal("0.000001"), rounding=ROUND_DOWN, context=Context(prec=6) | ||
| ) | ||
| # in case the user has changed the default precision or set traps. | ||
| with localcontext(DefaultContext) as ctx: |
There was a problem hiding this comment.
Small readability nit, I would import decimal then change this to
| with localcontext(DefaultContext) as ctx: | |
| with decimal.localcontext(DefaultContext) as ctx: |
Otherwise, from just reading this code, it is unclear that the localcontext is a decimal-related thing
Fixes #4213