feat(shared-data): change schema 2 to include user-defined volumes #18815
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18815 +/- ##
==========================================
+ Coverage 25.38% 25.47% +0.08%
==========================================
Files 3333 3334 +1
Lines 287669 288127 +458
Branches 36482 36538 +56
==========================================
+ Hits 73034 73390 +356
- Misses 214612 214714 +102
Partials 23 23
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
caila-marashaj
force-pushed
the
EXEC-762-user-defined-volumes
branch
from
8e03737 to
2eb5ef7
Compare
| test('the last entry for UserDefinedVolumes should be the max volume of the well at its depth', () => { | ||
| for (const well of Object.values(labwareDef.wells)) { | ||
| const wellGeometryId = well.geometryDefinitionId | ||
|
|
||
| if (wellGeometryId === undefined) return | ||
| if (labwareDef.innerLabwareGeometry == null) return | ||
| if (!('sections' in labwareDef.innerLabwareGeometry)) return | ||
| const wellGeometry = labwareDef.innerLabwareGeometry[wellGeometryId] | ||
| if (wellGeometry === undefined) return | ||
|
|
||
| const wellDepth = well.depth | ||
| const sectionList = wellGeometry.sections | ||
| const last_entry = sectionList[sectionList.length - 1] | ||
| expect(last_entry.height).toStrictEqual(wellDepth) | ||
| } | ||
| }) |
There was a problem hiding this comment.
sections, but I think we want it to check the new heightToVolumeMap instead, don't we?
Also heads up that the if (!('sections' in labwareDef.innerLabwareGeometry)) check looks like it's missing one level of nesting. Like 'sections' in labwareDef.innerLabwareGeometry[wellGeometryId]. (Except that sections would change to heightToVolumeMap, per the above.
There was a problem hiding this comment.
Separately, a stylistic suggestion: it looks like some of this null/undefined checking can be condensed with optional chaining and nullish coalescing.
| @@ -4434,6 +4450,130 @@ def test_virtual_find_height_and_volume( | |||
| assert height_estimate == volume_estimate == target_height_volume | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("use_mocks", [False]) | |||
| @given(target_height_volume_st=st.data()) | |||
| @settings(suppress_health_check=[HealthCheck.function_scoped_fixture]) | |||
There was a problem hiding this comment.
This HealthCheck that we're suppressing actually looks significant.
It's warning us that we're reusing the same labware_store and subject across Hypothesis examples, so they won't be isolated the way we usually expect.
We could probably get away with it here if we really needed to, but we ought to leave a comment in the test warning people not to do anything mutative.
Might be moot because of the larger points about this test, see below.
| def _expected_volume_result( | ||
| target_height: float, well_geometry: UserDefinedVolumes | ||
| ) -> float: | ||
| prev_height = 0.0 | ||
| prev_volume = 0.0 | ||
| if target_height == 0.0: | ||
| return 0.0 | ||
| for pair in well_geometry.heightToVolumeMap: | ||
| if target_height == pair.height: | ||
| return pair.volume | ||
| if target_height > prev_height and target_height < pair.height: | ||
| proportional_diff = (target_height - prev_height) / ( | ||
| pair.height - prev_height | ||
| ) | ||
| target_volume = ( | ||
| prev_volume + (pair.volume - prev_height) * proportional_diff | ||
| ) | ||
| return target_volume | ||
| prev_height = pair.height | ||
| prev_volume = pair.volume | ||
| raise ValueError("test function unable to find volume.") |
There was a problem hiding this comment.
At a glance, we have:
- Some Real Math in
frustum_helpers.py. - Some simple steering logic in
geometry.pythat picks which Real Math infrustum_helpersto use.
So, some options:
- Leaving all the production code as you have it, use Hypothesis to test the Real Math through the public interface of
frustum_helpers.py, and use Decoy to test thatgeometry.pycorrectly calls a mock offrustum_helpers.py.- This is how things were originally supposed to work, which is why most of the other tests in
test_geometry_view.pyare written this way.
- This is how things were originally supposed to work, which is why most of the other tests in
- Leaving all of the production code as you have it, use Hypothesis to test the Real Math through the public interface of
frustum_helpers.py, and leave the steering logic ingeometry.pyuntested.- This is acceptable IMO as long as the steering logic in
geometry.pyis trivial. The existingisinstance()check seems fine. Any kind of math would probably not be fine.
- This is acceptable IMO as long as the steering logic in
- Move the steering logic into
frustum_helpers.py. So its existing functions would become private, and in their place it would expose a single height-to-volume function and a single volume-to-height function that would automatically steer towards the appropriate implementation. Then, use Hypothesis to test everything through the public interface offrustum_helpers.py.
api/tests/opentrons/protocol_engine/state/test_geometry_view.py
Outdated
Show resolved
Hide resolved
| well_def = subject._labware.get_well_definition("labware-id-1", "A1") | ||
| well_geometry = subject._labware.get_well_geometry("labware-id-1", "A1") |
There was a problem hiding this comment.
We should generally try to avoid letting tests access private implementation details (_labware) of their subjects. Fortunately, it's easy to avoid in this case: you can just replace _labware with labware_store.
Might be moot because of the larger points about this test, see other comment.
caila-marashaj
force-pushed
the
EXEC-762-user-defined-volumes
branch
from
1937c5c to
859286c
Compare
Co-authored-by: Max Marrone <max@opentrons.com>
caila-marashaj
force-pushed
the
EXEC-762-user-defined-volumes
branch
from
7a03bc8 to
75b42fc
Compare
Overview
Liquid-level detection is out now, and its main blind-spot is labware that don't already have
InnerLabwareGeometrydefinitions in our shared-data repo. This is difficult for users to fill in on their own, as it comes from some pretty intensive measuring and modeling from our hardware department.As an alternative, we can give users/support people the option to add
UserDefinedVolumesto their labware's shared-data file, which will be a list of small dictionaries, each containing aheightentry and avolumeentry, like this:"innerLabwareGeometry": { "userVolumes": { "heightToVolumeMap": [ { "height": 0.5, "volume": 3 }, { "height": 2.0, "volume": 10 }, { "height": 10.4, "volume": 70 }, { "height": 30.8, "volume": 100 }, { "height": 59.3, "volume": 300 } ] }This pr enables this type of data to be used as input for the same liquid-level detection functionality that
InnerWellGeometrydefinitions use, likelabware.height_from_volumeandlabware.volume_from_height.InnerWellGeometrywill use the same geometric calculations we've been using before, and estimations usingUserDefinedVolumeswill use linear interpolations of the user-specified volumes and heights. This obviously won't be as accurate, but for a lot of labware, especially small labware, it won't matter a ton, so this is a good jumping off point I think.Changelog
InnerLabwareGeometryto be eitherInnerWellGeometryorUserDefinedVolumeslabware_definitionsas wellUserDefinedVolumesinfrustum_helpersgeometry.pydetermine the type ofInnerLabwareGeometryand call the right kind of calculationTesting
Hardware testing is coming up with a protocol that will allow users to automatically generate these
UserDefinedVolumesdata by dispensing known volumes into a well, and probing the liquid height in the well after dispensing. The math itself hasn't been rigorously tested yet, but once we get some more data to work with, we're going to use this protocol to come up withUserDefinedVolumesdata for labware with knownInnerWellGeometrydata, and compare the results ofheight_from_volumeandvolume_from_heighta bunch of times to evaluate the efficacy.