From 7ecfeebca74928975783eaf7c172b144c853bb5c Mon Sep 17 00:00:00 2001 From: CaseyBatten Date: Tue, 26 Sep 2023 16:07:29 -0400 Subject: [PATCH 1/7] Addition of error to prevent invalid disposal values Prevents the distribute functionality from accepting a disposal value equal to or greater than the maximum value of the instrument. --- api/src/opentrons/protocols/advanced_control/transfers.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/src/opentrons/protocols/advanced_control/transfers.py b/api/src/opentrons/protocols/advanced_control/transfers.py index 00f4a1ae02e..81619c2ab42 100644 --- a/api/src/opentrons/protocols/advanced_control/transfers.py +++ b/api/src/opentrons/protocols/advanced_control/transfers.py @@ -571,6 +571,12 @@ def _plan_distribute(self): .. Dispense air gap -> ...* """ + + if self._strategy.disposal_volume >= self._instr.max_volume: + raise ValueError( + "The Disposal Volume must be less than the Maximum Volume of the Instrument" + ) + # TODO: decide whether default disposal vol for distribute should be # pipette min_vol or should we leave it to being 0 by default and # recommend users to specify a disposal vol when using distribute. From e298cbc79ed4c8835745104c50b493e79d664120 Mon Sep 17 00:00:00 2001 From: CaseyBatten Date: Tue, 26 Sep 2023 17:07:36 -0400 Subject: [PATCH 2/7] Message update and _plan_distribute simplified Updated error message to be consistent with desired message, moved error raise to a validation function call to ensure _plan_distribute is not too complex --- .../opentrons/protocols/advanced_control/transfers.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/api/src/opentrons/protocols/advanced_control/transfers.py b/api/src/opentrons/protocols/advanced_control/transfers.py index 81619c2ab42..421b60b95b6 100644 --- a/api/src/opentrons/protocols/advanced_control/transfers.py +++ b/api/src/opentrons/protocols/advanced_control/transfers.py @@ -534,6 +534,12 @@ def _extend_source_target_lists( ] return sources, targets + def _check_valid_disposal_volume(self): + if self._strategy.disposal_volume >= self._instr.max_volume: + raise ValueError( + "The disposal volume must be less than the maximum volume of the pipette" + ) + def _plan_distribute(self): """ * **Source/ Dest:** One source to many destinations @@ -572,10 +578,7 @@ def _plan_distribute(self): """ - if self._strategy.disposal_volume >= self._instr.max_volume: - raise ValueError( - "The Disposal Volume must be less than the Maximum Volume of the Instrument" - ) + self._check_valid_disposal_volume() # TODO: decide whether default disposal vol for distribute should be # pipette min_vol or should we leave it to being 0 by default and From 41a8bd3bdc1763812b2b0ad3a93bbb803c337999 Mon Sep 17 00:00:00 2001 From: CaseyBatten Date: Wed, 27 Sep 2023 18:41:23 -0400 Subject: [PATCH 3/7] Adjustment to working volume from max and test case Working volume used to better reflect operating case of distribute, test case added to test transfers regarding distribute test --- .../opentrons/protocols/advanced_control/transfers.py | 10 ++++++---- .../protocols/advanced_control/test_transfers.py | 11 +++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/api/src/opentrons/protocols/advanced_control/transfers.py b/api/src/opentrons/protocols/advanced_control/transfers.py index 421b60b95b6..45ecd2f9f8a 100644 --- a/api/src/opentrons/protocols/advanced_control/transfers.py +++ b/api/src/opentrons/protocols/advanced_control/transfers.py @@ -534,10 +534,10 @@ def _extend_source_target_lists( ] return sources, targets - def _check_valid_disposal_volume(self): - if self._strategy.disposal_volume >= self._instr.max_volume: + def _check_valid_disposal_volume(self, disposal_volume, working_volume): + if disposal_volume >= working_volume: raise ValueError( - "The disposal volume must be less than the maximum volume of the pipette" + "The disposal volume must be less than the working volume of the pipette" ) def _plan_distribute(self): @@ -578,7 +578,9 @@ def _plan_distribute(self): """ - self._check_valid_disposal_volume() + self._check_valid_disposal_volume( + self._strategy.disposal_volume, self._instr._core.get_working_volume() + ) # TODO: decide whether default disposal vol for distribute should be # pipette min_vol or should we leave it to being 0 by default and diff --git a/api/tests/opentrons/protocols/advanced_control/test_transfers.py b/api/tests/opentrons/protocols/advanced_control/test_transfers.py index 71237286af7..6191faf2096 100644 --- a/api/tests/opentrons/protocols/advanced_control/test_transfers.py +++ b/api/tests/opentrons/protocols/advanced_control/test_transfers.py @@ -796,6 +796,17 @@ def test_all_options(_instr_labware): assert xfer_plan_list == exp1 +def test_invalid_disposal_volume_distribute(_instr_labware): + _instr_labware["ctx"].home() + # Supply the disposal volume assessment with the instrument followed by a disposal value equal to the max volume + with pytest.raises(ValueError): + tx.TransferPlan._check_valid_disposal_volume( + _instr_labware, + _instr_labware["instr"].hw_pipette["max_volume"], + _instr_labware["instr"].hw_pipette["max_volume"], + ) + + def test_oversized_distribute(_instr_labware): _instr_labware["ctx"].home() lw1 = _instr_labware["lw1"] From b85b711979b4ee3a480a888203fa3bb54dfd1051 Mon Sep 17 00:00:00 2001 From: CaseyBatten Date: Thu, 28 Sep 2023 13:02:19 -0400 Subject: [PATCH 4/7] Addition of air gap to the error throw, expansion of tests cases Test cases added/modified for disposal vol and air gaps, addition of calls to check for this issue across all transfer plans, assertion of check in validation --- .../protocols/advanced_control/transfers.py | 37 ++++++++-- .../advanced_control/test_transfers.py | 74 ++++++++++++++++++- 2 files changed, 99 insertions(+), 12 deletions(-) diff --git a/api/src/opentrons/protocols/advanced_control/transfers.py b/api/src/opentrons/protocols/advanced_control/transfers.py index 45ecd2f9f8a..8e2c19fe788 100644 --- a/api/src/opentrons/protocols/advanced_control/transfers.py +++ b/api/src/opentrons/protocols/advanced_control/transfers.py @@ -484,6 +484,11 @@ def _plan_transfer(self): -> Blow out -> Touch tip -> Drop tip* """ # reform source target lists + self._check_valid_volume_parameters( + self._strategy.disposal_volume, + self._strategy.air_gap, + self._instr._core.get_working_volume(), + ) sources, dests = self._extend_source_target_lists(self._sources, self._dests) plan_iter = self._expand_for_volume_constraints( self._volumes, @@ -534,12 +539,6 @@ def _extend_source_target_lists( ] return sources, targets - def _check_valid_disposal_volume(self, disposal_volume, working_volume): - if disposal_volume >= working_volume: - raise ValueError( - "The disposal volume must be less than the working volume of the pipette" - ) - def _plan_distribute(self): """ * **Source/ Dest:** One source to many destinations @@ -578,8 +577,10 @@ def _plan_distribute(self): """ - self._check_valid_disposal_volume( - self._strategy.disposal_volume, self._instr._core.get_working_volume() + self._check_valid_volume_parameters( + self._strategy.disposal_volume, + self._strategy.air_gap, + self._instr._core.get_working_volume(), ) # TODO: decide whether default disposal vol for distribute should be @@ -644,6 +645,7 @@ def _expand_for_volume_constraints( """Split a sequence of proposed transfers if necessary to keep each transfer under the given max volume. """ + assert max_volume > 0 for volume, target in zip(volumes, targets): while volume > max_volume * 2: yield max_volume, target @@ -691,6 +693,11 @@ def _plan_consolidate(self): *.. Aspirate -> Air gap -> Touch tip ->.. .. Aspirate -> .....* """ + self._check_valid_volume_parameters( + self._strategy.disposal_volume, + self._strategy.air_gap, + self._instr._core.get_working_volume(), + ) plan_iter = self._expand_for_volume_constraints( # todo(mm, 2021-03-09): Is it right to use _instr.max_volume here? # Why don't we account for tip max volume, disposal volume, or air @@ -854,6 +861,20 @@ def _map_volume(i): return [_map_volume(i) for i in range(total)] + def _check_valid_volume_parameters(self, disposal_volume, air_gap, working_volume): + if air_gap >= working_volume: + raise ValueError( + "The air gap must be less than the working volume of the pipette" + ) + elif disposal_volume >= working_volume: + raise ValueError( + "The disposal volume must be less than the working volume of the pipette" + ) + elif disposal_volume + air_gap >= working_volume: + raise ValueError( + "The sum of the air gap and disposal volume must be less than the working volume of the pipette" + ) + def _check_valid_well_list(self, well_list, id, old_well_list): if self._api_version >= APIVersion(2, 2) and len(well_list) < 1: raise RuntimeError( diff --git a/api/tests/opentrons/protocols/advanced_control/test_transfers.py b/api/tests/opentrons/protocols/advanced_control/test_transfers.py index 6191faf2096..8fccd2565c3 100644 --- a/api/tests/opentrons/protocols/advanced_control/test_transfers.py +++ b/api/tests/opentrons/protocols/advanced_control/test_transfers.py @@ -796,15 +796,81 @@ def test_all_options(_instr_labware): assert xfer_plan_list == exp1 +def test_invalid_air_gap_disposal_sum_distribute(_instr_labware): + _instr_labware["ctx"].home() + lw1 = _instr_labware["lw1"] + lw2 = _instr_labware["lw2"] + # Supply the disposal volume assessment with the instrument followed by a disposal value equal to the max volume + + options = tx.TransferOptions() + options = options._replace( + transfer=options.transfer._replace( + disposal_volume=_instr_labware["instr"].max_volume / 2, + air_gap=_instr_labware["instr"].max_volume / 2, + ) + ) + with pytest.raises(ValueError): + plan = tx.TransferPlan( + 700, + lw1.columns()[0][0], + lw2.rows()[0][1:3], + _instr_labware["instr"], + max_volume=_instr_labware["instr"].hw_pipette["max_volume"], + api_version=_instr_labware["ctx"].api_version, + mode="distribute", + options=options, + ) + list(plan) + + +def test_invalid_air_gap_distribute(_instr_labware): + _instr_labware["ctx"].home() + lw1 = _instr_labware["lw1"] + lw2 = _instr_labware["lw2"] + # Supply the disposal volume assessment with the instrument followed by a disposal value equal to the max volume + + options = tx.TransferOptions() + options = options._replace( + transfer=options.transfer._replace(air_gap=_instr_labware["instr"].max_volume) + ) + with pytest.raises(ValueError): + plan = tx.TransferPlan( + 700, + lw1.columns()[0][0], + lw2.rows()[0][1:3], + _instr_labware["instr"], + max_volume=_instr_labware["instr"].hw_pipette["max_volume"], + api_version=_instr_labware["ctx"].api_version, + mode="distribute", + options=options, + ) + list(plan) + + def test_invalid_disposal_volume_distribute(_instr_labware): _instr_labware["ctx"].home() + lw1 = _instr_labware["lw1"] + lw2 = _instr_labware["lw2"] # Supply the disposal volume assessment with the instrument followed by a disposal value equal to the max volume + + options = tx.TransferOptions() + options = options._replace( + transfer=options.transfer._replace( + disposal_volume=_instr_labware["instr"].max_volume + ) + ) with pytest.raises(ValueError): - tx.TransferPlan._check_valid_disposal_volume( - _instr_labware, - _instr_labware["instr"].hw_pipette["max_volume"], - _instr_labware["instr"].hw_pipette["max_volume"], + plan = tx.TransferPlan( + 700, + lw1.columns()[0][0], + lw2.rows()[0][1:3], + _instr_labware["instr"], + max_volume=_instr_labware["instr"].hw_pipette["max_volume"], + api_version=_instr_labware["ctx"].api_version, + mode="distribute", + options=options, ) + list(plan) def test_oversized_distribute(_instr_labware): From 344824bc2271c964201c3938414c3fbac252d96a Mon Sep 17 00:00:00 2001 From: CaseyBatten Date: Fri, 29 Sep 2023 12:19:20 -0400 Subject: [PATCH 5/7] consolidation of changes and improvements to test cases Adjustment of calls, change over to max volume control, formatting and improvement of test case formatting --- .../protocols/advanced_control/transfers.py | 33 +++++++++++-------- .../advanced_control/test_transfers.py | 20 +++++------ 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/api/src/opentrons/protocols/advanced_control/transfers.py b/api/src/opentrons/protocols/advanced_control/transfers.py index 8e2c19fe788..bb40ed1efa7 100644 --- a/api/src/opentrons/protocols/advanced_control/transfers.py +++ b/api/src/opentrons/protocols/advanced_control/transfers.py @@ -484,12 +484,12 @@ def _plan_transfer(self): -> Blow out -> Touch tip -> Drop tip* """ # reform source target lists + sources, dests = self._extend_source_target_lists(self._sources, self._dests) self._check_valid_volume_parameters( - self._strategy.disposal_volume, - self._strategy.air_gap, - self._instr._core.get_working_volume(), + disposal_volume=self._strategy.disposal_volume, + air_gap=self._strategy.air_gap, + max_volume=self._instr.max_volume, ) - sources, dests = self._extend_source_target_lists(self._sources, self._dests) plan_iter = self._expand_for_volume_constraints( self._volumes, zip(sources, dests), @@ -578,9 +578,9 @@ def _plan_distribute(self): """ self._check_valid_volume_parameters( - self._strategy.disposal_volume, - self._strategy.air_gap, - self._instr._core.get_working_volume(), + disposal_volume=self._strategy.disposal_volume, + air_gap=self._strategy.air_gap, + max_volume=self._instr.max_volume, ) # TODO: decide whether default disposal vol for distribute should be @@ -645,6 +645,9 @@ def _expand_for_volume_constraints( """Split a sequence of proposed transfers if necessary to keep each transfer under the given max volume. """ + # A final defense against an infinite loop. + # Raising a proper exception with a helpful message is left to calling code, + # because it has more context about what the user is trying to do. assert max_volume > 0 for volume, target in zip(volumes, targets): while volume > max_volume * 2: @@ -694,9 +697,9 @@ def _plan_consolidate(self): .. Aspirate -> .....* """ self._check_valid_volume_parameters( - self._strategy.disposal_volume, - self._strategy.air_gap, - self._instr._core.get_working_volume(), + disposal_volume=self._strategy.disposal_volume, + air_gap=self._strategy.air_gap, + max_volume=self._instr.max_volume, ) plan_iter = self._expand_for_volume_constraints( # todo(mm, 2021-03-09): Is it right to use _instr.max_volume here? @@ -861,16 +864,18 @@ def _map_volume(i): return [_map_volume(i) for i in range(total)] - def _check_valid_volume_parameters(self, disposal_volume, air_gap, working_volume): - if air_gap >= working_volume: + def _check_valid_volume_parameters( + self, disposal_volume: float, air_gap: float, max_volume: float + ): + if air_gap >= max_volume: raise ValueError( "The air gap must be less than the working volume of the pipette" ) - elif disposal_volume >= working_volume: + elif disposal_volume >= max_volume: raise ValueError( "The disposal volume must be less than the working volume of the pipette" ) - elif disposal_volume + air_gap >= working_volume: + elif disposal_volume + air_gap >= max_volume: raise ValueError( "The sum of the air gap and disposal volume must be less than the working volume of the pipette" ) diff --git a/api/tests/opentrons/protocols/advanced_control/test_transfers.py b/api/tests/opentrons/protocols/advanced_control/test_transfers.py index 8fccd2565c3..21f6c30dae9 100644 --- a/api/tests/opentrons/protocols/advanced_control/test_transfers.py +++ b/api/tests/opentrons/protocols/advanced_control/test_transfers.py @@ -802,9 +802,8 @@ def test_invalid_air_gap_disposal_sum_distribute(_instr_labware): lw2 = _instr_labware["lw2"] # Supply the disposal volume assessment with the instrument followed by a disposal value equal to the max volume - options = tx.TransferOptions() - options = options._replace( - transfer=options.transfer._replace( + options = tx.TransferOptions( + transfer=tx.Transfer( disposal_volume=_instr_labware["instr"].max_volume / 2, air_gap=_instr_labware["instr"].max_volume / 2, ) @@ -820,6 +819,7 @@ def test_invalid_air_gap_disposal_sum_distribute(_instr_labware): mode="distribute", options=options, ) + # Exhaust the iterator in case it raises the expected exception lazily. list(plan) @@ -829,9 +829,8 @@ def test_invalid_air_gap_distribute(_instr_labware): lw2 = _instr_labware["lw2"] # Supply the disposal volume assessment with the instrument followed by a disposal value equal to the max volume - options = tx.TransferOptions() - options = options._replace( - transfer=options.transfer._replace(air_gap=_instr_labware["instr"].max_volume) + options = tx.TransferOptions( + transfer=tx.Transfer(air_gap=_instr_labware["instr"].max_volume) ) with pytest.raises(ValueError): plan = tx.TransferPlan( @@ -844,6 +843,7 @@ def test_invalid_air_gap_distribute(_instr_labware): mode="distribute", options=options, ) + # Exhaust the iterator in case it raises the expected exception lazily. list(plan) @@ -853,11 +853,8 @@ def test_invalid_disposal_volume_distribute(_instr_labware): lw2 = _instr_labware["lw2"] # Supply the disposal volume assessment with the instrument followed by a disposal value equal to the max volume - options = tx.TransferOptions() - options = options._replace( - transfer=options.transfer._replace( - disposal_volume=_instr_labware["instr"].max_volume - ) + options = tx.TransferOptions( + transfer=tx.Transfer(disposal_volume=_instr_labware["instr"].max_volume) ) with pytest.raises(ValueError): plan = tx.TransferPlan( @@ -870,6 +867,7 @@ def test_invalid_disposal_volume_distribute(_instr_labware): mode="distribute", options=options, ) + # Exhaust the iterator in case it raises the expected exception lazily. list(plan) From e495aa991a4aa5d81aa560fdea0d0418cc12c9d7 Mon Sep 17 00:00:00 2001 From: CaseyBatten Date: Fri, 29 Sep 2023 12:32:16 -0400 Subject: [PATCH 6/7] Disable validation call under _plan_consolidate _plan_consolidate volume parameter check disabled --- .../opentrons/protocols/advanced_control/transfers.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/api/src/opentrons/protocols/advanced_control/transfers.py b/api/src/opentrons/protocols/advanced_control/transfers.py index bb40ed1efa7..759fd994512 100644 --- a/api/src/opentrons/protocols/advanced_control/transfers.py +++ b/api/src/opentrons/protocols/advanced_control/transfers.py @@ -696,11 +696,12 @@ def _plan_consolidate(self): *.. Aspirate -> Air gap -> Touch tip ->.. .. Aspirate -> .....* """ - self._check_valid_volume_parameters( - disposal_volume=self._strategy.disposal_volume, - air_gap=self._strategy.air_gap, - max_volume=self._instr.max_volume, - ) + # TODO: verify if _check_valid_volume_parameters should be re-enabled here + # self._check_valid_volume_parameters( + # disposal_volume=self._strategy.disposal_volume, + # air_gap=self._strategy.air_gap, + # max_volume=self._instr.max_volume, + # ) plan_iter = self._expand_for_volume_constraints( # todo(mm, 2021-03-09): Is it right to use _instr.max_volume here? # Why don't we account for tip max volume, disposal volume, or air From 5ac7a93dc0ae5fa99388ec659ed1d15bb0db3caf Mon Sep 17 00:00:00 2001 From: CaseyBatten Date: Tue, 3 Oct 2023 10:19:28 -0400 Subject: [PATCH 7/7] Message formatting consistency --- api/src/opentrons/protocols/advanced_control/transfers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/opentrons/protocols/advanced_control/transfers.py b/api/src/opentrons/protocols/advanced_control/transfers.py index 759fd994512..8f513a3afff 100644 --- a/api/src/opentrons/protocols/advanced_control/transfers.py +++ b/api/src/opentrons/protocols/advanced_control/transfers.py @@ -870,15 +870,15 @@ def _check_valid_volume_parameters( ): if air_gap >= max_volume: raise ValueError( - "The air gap must be less than the working volume of the pipette" + "The air gap must be less than the maximum volume of the pipette" ) elif disposal_volume >= max_volume: raise ValueError( - "The disposal volume must be less than the working volume of the pipette" + "The disposal volume must be less than the maximum volume of the pipette" ) elif disposal_volume + air_gap >= max_volume: raise ValueError( - "The sum of the air gap and disposal volume must be less than the working volume of the pipette" + "The sum of the air gap and disposal volume must be less than the maximum volume of the pipette" ) def _check_valid_well_list(self, well_list, id, old_well_list):