Skip to content

Comments

Improve learn_rounding_digits precision#964

Merged
fealho merged 5 commits intomainfrom
fix-rounding
Mar 28, 2025
Merged

Improve learn_rounding_digits precision#964
fealho merged 5 commits intomainfrom
fix-rounding

Conversation

@fealho
Copy link
Member

@fealho fealho commented Mar 27, 2025

CU-86b455jpu, Resolve https://github.com/datacebo/SDV-Enterprise/issues/1107.

A float64 has 15-17 digits of precision. Anything after that should be disconsidered. For example:

f"{np.float64(0.1):.15f}" -> '0.100000000000000'
f"{np.float64(0.1):.20f}" -> '0.10000000000000000555'

These 15-17 digits include the integer part of the number, for example:

f"{np.float64(100.1):.12f}" -> '100.100000000000'
f"{np.float64(100.1):.15f}" -> '100.099999999999994'

The previous implementation always attempted to round to MAX_DECIMALS - 1 digits, ie 14 decimal places, which could fail for cases like above where there's an integer component to the number:

np.float64(944343.1221).round(13) -> 944343.1220999999
np.float64(944343.1221).round(12) -> 944343.1221

This PR fixes this issue by rounding to MAX_DECIMALS - num_integer_digits instead, ensuring the situation above never happens.

@fealho fealho force-pushed the fix-rounding branch 2 times, most recently from e307458 to b9de0c0 Compare March 27, 2025 16:33
@codecov
Copy link

codecov bot commented Mar 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e7bacd2) to head (b103696).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #964   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         2352      2353    +1     
=========================================
+ Hits          2352      2353    +1     
Flag Coverage Δ
integration 83.08% <100.00%> (+<0.01%) ⬆️
unit 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fealho fealho force-pushed the fix-rounding branch 4 times, most recently from f5b6516 to bd56883 Compare March 27, 2025 17:28
@fealho fealho changed the title Fix learn_rounding_digits Increase learn_rounding_digits precision Mar 28, 2025
@sdv-team
Copy link
Contributor

@fealho fealho marked this pull request as ready for review March 28, 2025 05:18
@fealho fealho requested a review from a team as a code owner March 28, 2025 05:18
@fealho fealho requested review from R-Palazzo, frances-h and pvk-developer and removed request for a team and frances-h March 28, 2025 05:18
Copy link
Contributor

@R-Palazzo R-Palazzo left a comment

Choose a reason for hiding this comment

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

Have you tried the changes on the colab notebook of the issue?

@fealho fealho requested a review from R-Palazzo March 28, 2025 15:23
Copy link
Contributor

@R-Palazzo R-Palazzo left a comment

Choose a reason for hiding this comment

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

LGTM!
Great test to understand and double check everything 👍

@fealho
Copy link
Member Author

fealho commented Mar 28, 2025

Have you tried the changes on the colab notebook of the issue?

Added a notebook showing it fixes the original problem here.

@fealho fealho requested a review from pvk-developer March 28, 2025 16:49
@fealho fealho changed the title Increase learn_rounding_digits precision Improve learn_rounding_digits precision Mar 28, 2025
@fealho fealho requested a review from frances-h March 28, 2025 20:37
@fealho fealho merged commit 2eb7dff into main Mar 28, 2025
55 checks passed
@fealho fealho deleted the fix-rounding branch March 28, 2025 21:23
@fealho fealho self-assigned this Mar 31, 2025
@fealho fealho added this to the 1.16.0 milestone Mar 31, 2025
@amontanez24 amontanez24 removed this from the 1.16.0 milestone Mar 31, 2025
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.

6 participants