connorferster / forallpeople

Python SI units library: your 'daily driver' for calculations.
Apache License 2.0
299 stars 40 forks source link

Comparison of unit values #27

Closed amzz closed 2 years ago

amzz commented 3 years ago

It looks like there might be a bug in the comparison

>>> 1e-6 * m ==  0 * m
False
>>> 1e-6 * m >  0 * m
True
>>> 1e-9 * m ==  0 * m
True
>>> 1e-9 * m > 0 * m
False
connorferster commented 3 years ago

Hi @amzz,

Thanks for bringing this up. This is (undocumented) expected behaviour due to an implementation detail.

When I implemented the ability to "scale" SI units by a factor, I implemented the scaling factor in floating point arithmetic. When scaled units are multiplied together, their .factor attributes are also multiplied and stored as the .factor attribute in the new Physical (e.g. ft * inch * lb). Over a certain amount of operations, the floating point arithmetic begins to progressively introduce errors in the tail-end decimal places (e.g. 14th, 12th, 9th decimal places). As the error progresses then, eventually, the factor does not match the stored factors in the Environment.defined_units dict and the scaling of the Physical is no longer recognized. To deal with this, I implemented a maximum tolerance of six decimal places, which is what you are observing in your example.

For my own work, six decimal places is plenty so I have not worried about it yet! However, I have been thinking about how this is a flaw that would be good to fix. I think I might use the Decimal library for the .factor attribute and that would likely prevent this progressive loss in accuracy. I played with the idea originally of making everything in forallpeople based on Decimal but this quickly did not work. However, I think I can contain its use to just dealing with the .factor attributes.

I expect to implement this in the next version of forallpeople. Don't have a timeline for it yet because I am currently consumed with other projects but I expect within the next few months.

connorferster commented 2 years ago

@amzz This should be fixed now in the new version. The factor attribute is no stored as a Fraction to retain as much precision as possible.