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.

Sign up for GitHub

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

Jump to bottom

Construct fraction and eliminate common terms when calculating conversion factor to increase accuracy #2108

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Construct fraction and eliminate common terms when calculating conversion factor to increase accuracy #2108

wants to merge 7 commits into from

Conversation

nneskildsf
Copy link
Contributor

@nneskildsf nneskildsf commented Jan 3, 2025 •
edited
Loading

This change increases accuracy for unit conversions involving fractions which do not have real number representations.
Some conversions take long winding paths where premature evaluation of fractions lead to numerical errors.

Conversions using pint 0.24.4:

Using regular float:
1 inch -> thou: 1000.0 thou
1 L/h -> L/day: 23.999999999999996 liter / day

Using decimal:
1 inch -> thou: 999.9999999999999999999999996
1 L/h -> L/day: 24.00000000000000000000000001 liter / day

Using this PR:

Using regular float:
1 inch -> thou: 1000.0 thou
1 L/h -> L/day: 24.0 liter / day

Using decimal:
1 inch -> thou: 1.0E+3 thou
1 L/h -> L/day: 24.0 liter / day

Example

Take for instance conversion from inch to thou. The factor is 1000 so conversion should be lossless. However, the conversion process is actually inch->yard->meter->yard->inch->thou. The fraction is built one multiplication at a time and this incurs a loss of precision.
The calculation is:
factor = 1 * (1/36) * (1/0.9144) * 0.9144 * 36 * 1000

This change first builds the fraction, then eliminates common terms in the fraction and finally it is evaluated numerically. The fraction is built and elimination performed resulting in factor = 1000.

@nneskildsf
Use division instead of exponentiation to perform division
fa4ed4b
@nneskildsf
Update CHANGES
4241c0f
@nneskildsf
Update test_issues.py
34aacbb
@nneskildsf
Update registry.py
e0b9ace 
Calculate factor by first constructing the scaling fraction, eliminating terms that appear in numerator and denominator and then finally evaluating the fraction.
This was referenced Jan 3, 2025
Open
Open
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jan 4, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #2108 will not alter performance

Comparing nneskildsf:fix-decimal-loss-of-precision (cbdfdc5) with master (74b7086)

Summary

✅ 437 untouched benchmarks

@andrewgsavage
Copy link
Collaborator

This looks good.
Do you have any ideas as to why tests test_to_compact_fraction and test_issue1963 are failing?

I wonder if test_to_compact_fraction is related to to using fraction in a a non_int_type = float ureg

@nneskildsf
Copy link
Contributor Author

nneskildsf commented Jan 4, 2025 •
edited
Loading

This looks good. Do you have any ideas as to why tests test_to_compact_fraction and test_issue1963 are failing?

I wonder if test_to_compact_fraction is related to to using fraction in a a non_int_type = float ureg

Thank you for the encouragement.

test_to_compact_fraction failed because 1.0000000000000002 kilosecond / meter is not 1000 second/meter. Currently, the factor is calculated by multiplying 1 with the numerator factors exponentiated appropriately and then dividing by the denominator factors likewise exponentiated appropriately.
Thus the factor is evaluated as:
0.001/(1e-6)/1000.0 which on Python 3.13 Linux indeed returns 1.0000000000000002, hence the failed test.

If I instead use the Pint 0.24.4 method of calculating only using multiplication and setting negative sign for the exponent for denominator terms then the factor is evaluated as:
0.001*(1e-6)**-1*1000.0**-1 which does return 1.0 as expected.

test_issue1963 fails due to the same reason. Conversion of 1e2 * 100 ppm to permille involves evaluating 1e2 * 1e-6/0.001 which returns 0.09999999999999999 and not 0.1.
Interestingly, the test case evaluates a == 1e2 * b which is False (hence the failure) but 1e2 * b == a is True.
Using only multiplication as described above, both a == 1e2 * b and 1e2 * b == a evaluate True as expected.

I have updated the PR to use multiplication only.

@andrewgsavage
Copy link
Collaborator

I'll merge this before the next release. Leaving it open until then for others to comment on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Converting a Decimal number results in a loss of precision
3 participants
@nneskildsf @andrewgsavage @eskildsf