feat(api): proper LiquidHandlingPropertyByVolume class and validation for setting liquid class properties #16725
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #16725 +/- ##
=======================================
Coverage 92.43% 92.43%
=======================================
Files 77 77
Lines 1283 1283
=======================================
Hits 1186 1186
Misses 97 97
Flags with carried forward coverage won't be shown. Click here to find out more. |
| # numpy interp does not automatically sort the points used for interpolation, so | ||
| # here we are sorting them by the keys (volume) and then using zip to get two | ||
| # tuples in the correct order | ||
| sorted_volumes, sorted_values = zip(*sorted(self._properties_by_volume.items())) |
There was a problem hiding this comment.
What if we we make a separate function for fetching the sorted dict and add caching to it so that this function doesn't end up doing sorts again and again on the same set of data? Slight efficiency improvement
| # here we are sorting them by the keys (volume) and then using zip to get two | ||
| # tuples in the correct order | ||
| sorted_volumes, sorted_values = zip(*sorted(self._properties_by_volume.items())) | ||
| return float(interp(validated_volume, sorted_volumes, sorted_values)) |
There was a problem hiding this comment.
Also, not sure if the interpolator goes through the trouble of actually doing the interpolation even for values that are readily available in the dictionary. If it does, then I think it'll be a bit better to first check if the volume exists in the dictionary and interpolate only if it doesn't exist.
| def validate_coordinates(value: Sequence[float]) -> Tuple[float, float, float]: | ||
| """Ensure value is a valid sequence of 3 floats and return a tuple of 3 floats.""" | ||
| if len(value) != 3: | ||
| raise ValueError("Coordinates must be a sequence of length 3") |
There was a problem hiding this comment.
| raise ValueError("Coordinates must be a sequence of length 3") | |
| raise ValueError("Coordinates must be a sequence of exactly three numbers") |
maybe?
|
|
||
| @pytest.mark.parametrize("value", [1.0, -1.0, -1]) | ||
| def test_ensure_positive_int_raises(value: Union[int, float]) -> None: | ||
| """It should return a positive integer.""" |
There was a problem hiding this comment.
| """It should return a positive integer.""" | |
| """It should raise if value is not a positive integer.""" |
|
|
||
| @pytest.mark.parametrize("value", [-1, -1.0, float("inf"), float("-inf"), float("nan")]) | ||
| def test_ensure_positive_float_raises(value: Union[int, float]) -> None: | ||
| """It should return a positive float.""" |
There was a problem hiding this comment.
| """It should return a positive float.""" | |
| """It should raise if value is not a positive float.""" |
|
|
||
| @pytest.mark.parametrize("value", [[1, 2.0], [1, 2.0, 3.3, 4.2], ["1", 2, 3]]) | ||
| def test_validate_coordinates_raises(value: Sequence[Union[int, float, str]]) -> None: | ||
| """It should return a positive integer.""" |
There was a problem hiding this comment.
| """It should return a positive integer.""" | |
| """It should raise if value is not a positive integer.""" |
| return self._default | ||
|
|
||
| @property | ||
| def properties_by_volume(self) -> Dict[Union[float, str], float]: |
There was a problem hiding this comment.
Hmm.. I wonder if there's a more intuitive way of representing this. Ideally would be good to avoid a call like aspirate.flow_rate_by_volume.properties_by_volume.
I guess maybe calling this property/ function something like get_dict() might also be an option
|
|
||
|
|
||
| class LiquidHandlingPropertyByVolume: | ||
| def __init__(self, properties_by_volume: Dict[str, float]) -> None: |
There was a problem hiding this comment.
Explain? The argument is Dict[str, float], but some of the dict keys are strings and some are numbers?
Overview
Closes AUTH-837
This PR adds a proper
LiquidHandlingPropertyByVolumeclass, replacing the previous dictionary of string keys to float values that was the literal representation of the liquid class schema. Now it uses that dictionary to make adefaultvalue as well as functions to take any volume and interpolate the proper volume based on that. Methods are also there for setting new points for volume and deleting existing points.The other part of this PR is adding validation for the different settable values in liquid class that would require validating before hand. This includes boolean values, floating points, positive ints and floats, and xyz coordinates.
Changelog
LiquidHandlingPropertyByVolumeclass to handle interpolation for these properties and adding/deleting pointsReview requests
Should all the new validators live in the general PAPI
validation.pyclass or should they exist elsewhere?Risk assessment
Low.