Make sure to use the default decimal context in our code#4231
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #4231 +/- ##
=======================================
Coverage 80.31% 80.32%
=======================================
Files 142 142
Lines 15930 15933 +3
Branches 2725 2725
=======================================
+ Hits 12794 12798 +4
Misses 2262 2262
+ Partials 874 873 -1
|
| # 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.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
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
|
We will remove the |
3 participants
Fixes #4213