Comparison tools for UnitScalars/UnitArrays #85
Conversation
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
==========================================
- Coverage 60.87% 60.84% -0.04%
==========================================
Files 75 77 +2
Lines 3208 3264 +56
Branches 368 376 +8
==========================================
+ Hits 1953 1986 +33
- Misses 1163 1186 +23
Partials 92 92
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
==========================================
+ Coverage 60.87% 61.32% +0.44%
==========================================
Files 75 77 +2
Lines 3208 3266 +58
Branches 368 377 +9
==========================================
+ Hits 1953 2003 +50
- Misses 1163 1171 +8
Partials 92 92
Continue to review full report at Codecov.
|
|
CI happy (though coverage is down, I will fix that). Ready for feedback. cc @JCorson |
scimath/units/compare_units.py
Outdated
| from scimath.units.unit import InvalidConversion | ||
|
|
||
|
|
||
| def unit_scalars_almost_equal(x1, x2, eps=1e-9): |
There was a problem hiding this comment.
I think we really want eps to be a quantity of the same unit class as x1 and x2 here, and I'd suggest removing the default value and making it a required argument. It just doesn't make sense conceptually to ask whether two lengths (for example) are equal up to two decimal places, while it absolutely makes sense to ask whether two lengths are equal to within a margin of 2cm, or 20nm, or 1/8 inch, or ...
It's particularly unclear what comparing to 1e-9 would mean in the case that x1 and x2 are comparable, but have different actual units, and the implementation here looks as though it would give asymmetric results:
>>> x = UnitScalar(1.0, units="um")
>>> y = UnitScalar(1002, units="nm")
>>> unit_scalars_almost_equal(x, y, eps=1e-2)
True
>>> unit_scalars_almost_equal(y, x, eps=1e-2)
False
There was a problem hiding this comment.
Thanks for the valuable feedback @mdickinson . I totally agree with you on eps needing to be unitted, and for it to be a required argument. I would only suggest to allow for eps to be a float if the 2 values have the same unit: seems reasonable to you? I will push an update...
There was a problem hiding this comment.
I'd go further, and only allow use of a float eps if both quantities being compared are actually unitless. Otherwise you're again comparing a unitful quantity with a unitless quantity, which seems like a category error to me. It would seem wrong to me if a given unit_scalars_almost_equal call passes for some length length1 and fails for a length2 that represents exactly the same length (so length1 == length2 gives True), but happens to use different units.
There was a problem hiding this comment.
IOW, I'd want the check to be checking a property of the length itself, not a property of the representation of that length.
There was a problem hiding this comment.
Thanks, I see your point. At the same time, I would like to keep it simple to use. What you propose would lead to this for the usage
from scimath.units.testing.assertion_utils import assert_unit_scalar_almost_equal
from scimath.units.api import UnitScalar
assert_unit_scalar_almost_equal(x, y, eps=UnitScalar(1e-12, units="SOME UNIT HERE"))
is quite verbose. What I meant to do with the current implementation is:
assert_unit_scalar_almost_equal(x, y, eps=UnitScalar(1e-12, units=x.units))
What about automatically treating a float eps like UnitScalar(eps, units=x.units) inside unit_scalars_almost_equal? Or change eps to a non-dimensional "rtol" which would be compared to float(abs(x1-x2)/abs(x2)) (after unit conversion) similar to https://docs.scipy.org/doc/numpy-1.13.0/reference/generated/numpy.allclose.html ?
|
Following my last proposal, I have converted the (meaningless) absolute comparison to a relative comparison, and renamed the similar to https://docs.scipy.org/doc/numpy-1.13.0/reference/generated/numpy.allclose.html Feedback welcome @mdickinson . |
|
Bumping this back up... |
1 similar comment
|
Bumping this back up... |
Add assertion functions and functions returning booleans to test whether 2 unitted objects (scalars or arrays) are "almost equal".
Feedback welcome! Maybe that extra nesting isn't all that useful after all...