Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix learn_rounding_scheme for more than 14 digits #591

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

fealho
Copy link
Member

@fealho fealho commented Dec 5, 2022

Resolve #556.

@fealho fealho requested a review from a team as a code owner December 5, 2022 17:53
@fealho fealho requested review from pvk-developer and amontanez24 and removed request for a team December 5, 2022 17:53
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (8290d4a) compared to base (a861edc).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #591   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         1467      1474    +7     
=========================================
+ Hits          1467      1474    +7     
Impacted Files Coverage Δ
rdt/transformers/numerical.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I believe we used to think that returning None meant we didn't round at all, but now we have the line of code that says data.round(self._rounding_digits or 0). I think we agreed to round to the MAX_DECIMALS but I think it would also be ok to just not round in this case.

@fealho fealho merged commit 2f39353 into master Dec 7, 2022
@fealho fealho deleted the issue-556-rounding-max-digits branch December 7, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Float formatter learn_rounding_scheme doesn't work on all digits
4 participants