Repository navigation
Only convert unit when nessecary - #361
damskii9992 wants to merge 2 commits into
Conversation
damskii9992
added
[scope] maintenance
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #361 +/- ##
===========================================
- Coverage 92.62% 92.53% -0.10%
===========================================
Files 47 47
Lines 3350 3374 +24
===========================================
+ Hits 3103 3122 +19
- Misses 247 252 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
It would also be nice to have some benchmark numbers in the PR body, since this is what you're aiming at.
Also the name is misleading, there's nothing about dependent parameters here, it's about parameter arithmetics
| return NotImplemented | ||
| return Parameter.from_scipp(full_value=new_full_value, min=min_value, max=max_value) | ||
|
|
||
| def __sub__(self, other: DescriptorNumber | Parameter | numbers.Real) -> Parameter: |
There was a problem hiding this comment.
Did you forget to change __sub__?
| min_value = other.value - self.max | ||
| max_value = other.value - self.min | ||
| self._convert_unit(original_unit) | ||
| if other.unit != self.unit: |
There was a problem hiding this comment.
isn't other.unit != self.unit check expensive, since it builds actual strings?
wouldn't a._scalar.unit != b._scalar_unit using scipp Unit be much faster?
There was a problem hiding this comment.
Indeed it would be almost 10x faster
(era) C:\Users\piotr\projects\easy\ERA\core>python -m timeit -s "from easyscience.variable import DescriptorNumber as D; a=D(1.0,unit='m'); b=D(2.0,unit='m')" "a.unit != b.unit"
50000 loops, best of 5: 9.95 usec per loop
(era) C:\Users\piotr\projects\easy\ERA\core>python -m timeit -s "from easyscience.variable import DescriptorNumber as D; a=D(1.0,unit='m'); b=D(2.0,unit='m')" "a._scalar.unit != b._scalar.unit"
200000 loops, best of 5: 1.43 usec per loop
| if other.unit != self.unit: | ||
| original_unit = other.unit | ||
| try: | ||
| other._convert_unit(self.unit) | ||
| except UnitError: | ||
| raise UnitError( | ||
| f'Values with units {self.unit} and {other.unit} cannot be added' | ||
| ) from None |
There was a problem hiding this comment.
Those checks are identical in all methods. Refactor out?
| if isinstance(other, Parameter) | ||
| else self.max + other.value | ||
| ) | ||
| return Parameter.from_scipp(full_value=new_full_value, min=min_value, max=max_value) |
There was a problem hiding this comment.
remove this. We are already returning in line 1011
|
NVM I just noticed you put this PR in draft |
This PR only converts the unit in the add/sub dunder methods when the unit actually needs to be converted, saving some calculation time.