Refactors Quantity to have a standard error property#215
Conversation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
No quality gates enabled for this code.
See analysis details in CodeScene
Quality Gate Profile: Custom Configuration
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
rprospero
left a comment
rprospero
left a comment
There was a problem hiding this comment.
Just a bit of confusion on two points.
| self._variance_cache = self.history.variance_propagate(self.units) | ||
|
|
||
| return self._variance_cache | ||
| return self._variance_cache**0.5 |
There was a problem hiding this comment.
This function surprised me slightly and is making me think that I've misunderstood the PR. I'd rather assumed that the standard_error property would just return the value of _standard_error, as defined in line 1573.
There was a problem hiding this comment.
Yes, this is the case for the standard_error property of the Quantity object, defined on line 1607. Here is the standard_error property of the DerivedQuantity object. Note that the __init__ for this class is:
class DerivedQuantity[QuantityType](Quantity[QuantityType]):
def __init__(self, value: QuantityType, units: Unit, history: QuantityHistory):
super().__init__(value, units, standard_error=None)
self.history = history
self._variance_cache = None
self._has_error = history.has_error()
i.e., we call the __init__ for a Quantity with standard_error = None. This is because for a DerivedQuantity we need to propagate errors from the quantity history, rather than read the property.
| @property | ||
| def variance(self) -> "Quantity": | ||
| """Get the variance of this object""" | ||
| return self.standard_error**2 |
There was a problem hiding this comment.
It's a little odd that variance returns standard_error**2, but the standard_error property returns self._variance_cache**0.5. Wouldn't it be better to cut out the middle man and just return self._variance_cache here?
There was a problem hiding this comment.
We can't do this here, because the standard error is defined differently for Quantity compared to DerivedQuantity, as noted above. We could write a variance property for the DerivedQuantity defined as you suggest, but I felt that the code duplication wasn't justified in this case despite the simpler operation. Happy to hear feedback on that point though.
2 participants
This PR refactors
quantity.pyto ensure Quantity objects have astandard_errorproperty, with thevarianceproperty being simplystandard_error**2. This makes it easier to access errors in other parts of the SasData code.I propose merging this PR before #207, and then rebasing and rewriting that PR to incorporate the changes in this one.