-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add correct comparison support for Scalar #31
Add correct comparison support for Scalar #31
Conversation
Codecov Report
@@ Coverage Diff @@
## master #31 +/- ##
==========================================
+ Coverage 97.47% 97.55% +0.08%
==========================================
Files 53 53
Lines 9270 9339 +69
==========================================
+ Hits 9036 9111 +75
+ Misses 234 228 -6 |
954f5c6
to
11e62f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a CHANGELOG entry
1fcda5a
to
6479e66
Compare
src/barril/units/_scalar.py
Outdated
@@ -242,8 +242,8 @@ def GetFormatted(self, unit=None, value_format=None): | |||
def __eq__(self, other): | |||
return ( | |||
type(self) is type(other) | |||
and self._value == other.value | |||
and self._quantity == other._quantity | |||
and self._value == other.GetValue(self.unit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to floating point rounding self._value == other.GetValue(self.unit)
this could lead to situations where (a == b) != (b == a)
.
I think this should agree on the unit used for comparison. Maybe the one composed by composing quantities base units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored to do the comparison always using DefaultUnit
:
def __eq__(self, other):
return (
type(self) is type(other)
and self._quantity._category == other._quantity._category
and self._GetValueInDefaultUnit() == other._GetValueInDefaultUnit()
)
def _GetValueInDefaultUnit(self):
try:
return self.GetValue(
self._unit_database.GetDefaultUnit(self._quantity._category)
)
except InvalidQuantityTypeError:
return self._value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scalars with the same category could have different units...
I don't think returning the raw value is a good option in InvalidQuantityTypeError
.
Maybe return (self._value, self._quantity._unit)
? If you do something like this I recommend a docstring stating that.
I just noted that this is also used on almost equal. Sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, ok. I've took you suggestion, but with a slightly modification, take a look.
8638425
to
7dbe280
Compare
d9114e1
to
0e4c04a
Compare
28c6e14
to
e4657c5
Compare
Modified comparison behavior of ``Scalar``. The previous behavior assumes that ``Scalar(1, "m") != Scalar(100, "cm")`` and not it has been changed to ``Scalar(1, "m") == Scalar(100, "cm")``. This may affect users that rely on the previous behavior. BARRIL-26
…lar passing a unit
1bbd2ae
to
e982896
Compare
There are codebases that rely on the current comparison behavior. Decided not to merge it at the moment. |
This PR attempts to fix
<=
and>=
comparisons withScalar
Fix #26