Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(api): proper LiquidHandlingPropertyByVolume class and validation for setting liquid class properties #16725
feat(api): proper LiquidHandlingPropertyByVolume class and validation for setting liquid class properties #16725
Changes from 9 commits
538f030
b88b6f7
6d4ff73
55f936d
f37d74d
f436ffd
c842f0b
2d5c9ba
8603f1c
559bde7
91ce7c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How does the
default
play into this function?Also, for someone not so familiar with numpy, and not so familiar with the expected behavior of the machines, it'd be helpful to have a comment that just outright states that you're trying to do a piecewise linear interpolation.
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.
Hm, I'm still confused about the behavior of
default
. Let's say someone gives you this config without any non-default points:Is
get_for_volume()
expected to return 5?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.
FYI, forcing the control points on the interpolation curve to be strictly positive makes life annoying for users. Like, if I want to configure a rate of 3 per uL, the easiest way to do that would be:
But if you reject
0 uL
because it's not a positive float, then it's harder to set up the curve.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.
0 is an allowed value for this, I can rename this validation function if need be since I know zero isn't a positive number.
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 would prefer that, but otherwise this PR looks fine. I can't think of a good short name for "ensure positive or zero" though.
I will note that in the future, there will probably be things that we need to be strictly positive (flow rates, dimensions) and some things that can be positive-or-zero (like the control points for interpolation). If you claim the
ensure_positive_float()
name now, someone who wants to implement "ensure strictly positive float" later will have to work around your naming.