chore(refactor): Refactor and cleanup volumetric scripts #13071
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## edge #13071 +/- ##
==========================================
- Coverage 72.11% 63.88% -8.23%
==========================================
Files 1531 1616 +85
Lines 50773 27976 -22797
Branches 3144 7219 +4075
==========================================
- Hits 36614 17873 -18741
+ Misses 13650 8361 -5289
- Partials 509 1742 +1233
Flags with carried forward coverage won't be shown. Click here to find out more. |
sfoster1
approved these changes
Jul 10, 2023
There was a problem hiding this comment.
This is looking really solid. I think we could make it a little better by splitting some massive triple for loops into functions, but really my most specific rec is: we should get replace print and input with a call to that ui module. It'll really help if we ever want to add fanciness there.
| @@ -235,8 +229,9 @@ def run_photometric( | |||
| deck_version="2", | |||
| extra_labware=custom_defs, | |||
| ) | |||
| union_cfg: Union[PhotometricConfig, GravimetricConfig] | |||
There was a problem hiding this comment.
nit: we could factor this into like
def config_from_args(args: argparse.Namespace, context: Whatever_Ctx_is) -> Union[PhotometricConfig, GravimetricConfig]`
which means we wouldn't have to do the temporarily-unbound variable thing and could test it easily if we ever added tests
| tips=get_tips(_ctx, pipette, all_channels=all_channels_same_time), | ||
| ) | ||
|
|
||
| if args.photometric: |
There was a problem hiding this comment.
If we added an element to each config called kind: Literal['photometric'] and kind: Literal['gravimetric'] then you'd be able to have this be a discriminated union which means you could do if cfg.kind == 'photometric' and mypy would figure it out. You could also bind the run function into the config object.
| labware_on_scale: Labware, | ||
| measure_height: float, | ||
| ) -> Tuple[float, float]: | ||
| if not cfg.blank or cfg.inspect: |
There was a problem hiding this comment.
recommend factoring the body of each if/else into its own function to save an indent level
| False, | ||
| measure_height=measure_height, | ||
| ) | ||
| for volume in trials.keys(): | ||
| actual_asp_list_all = [] |
There was a problem hiding this comment.
Might be worth getting these for loops to each be their own function
| pipette.pick_up_tip(location) | ||
| # NOTE: the accuracy-adjust function gets set on the Pipette | ||
| # each time we pick-up a new tip. | ||
| if getattr(cfg, "increment", False): |
|
|
||
| # NOTE: 8ch QC testing means testing 1 channel at a time, | ||
| # so we need to decrease the pick-up current to work with 1 tip. | ||
| if pipette.channels == 8 and not getattr(cfg, "increment", False): |
ryanthecoder
force-pushed
the
refactor-and-clean-volumetric-scripts
branch
3 times, most recently
from
50dc526
to
9f5420f
Compare
ryanthecoder
force-pushed
the
refactor-and-clean-volumetric-scripts
branch
from
eb5b6f0
to
5c21325
Compare
andySigler
reviewed
Jul 18, 2023
andySigler
reviewed
Jul 18, 2023
| hwapi = get_sync_hw_api(ctx) | ||
| mnt = OT3Mount.LEFT if cfg.pipette_mount == "left" else OT3Mount.RIGHT | ||
| hwpipette: Pipette = hwapi.hardware_pipettes[mnt.to_mount()] | ||
| hwpipette.pick_up_configurations.current = 0.2 |
There was a problem hiding this comment.
we should do something to call this out more, maybe pull its definition up to somewhere more visible in a followup PR
andySigler
reviewed
Jul 18, 2023
| blank=blank, | ||
| inspect=cfg.inspect, | ||
| mix=cfg.mix, | ||
| stable=False, |
andySigler
reviewed
Jul 18, 2023
andySigler
reviewed
Jul 18, 2023
andySigler
reviewed
Jul 18, 2023
andySigler
approved these changes
Jul 18, 2023
ryanthecoder
force-pushed
the
refactor-and-clean-volumetric-scripts
branch
from
2577b15
to
21915e9
Compare
ryanthecoder
added a commit
that referenced
this pull request
Jul 26, 2023
* Revert "Merge latest volumetric changes (#13100)" This reverts commit 773b8a4. * Revert "fix(hardware-testing): Remove tips using them for finding liquid height (#13064)" This reverts commit cc0f509. * Revert "support resupplying tipracks during a 96ch increment test run (#13065)" This reverts commit 965f02d. * Pull out all of the common functions between gravimetric and photometric * move trial generation out of excecute * combine the load pipette logic * pull gantry speed out of the photometric config * refactor out load tipracks * remove unused method * fix issues from rebasing * fixups from rebase and formating * refactor out trial building from the photometric script * add additional test targets * pull out the common elements of trials into a seperate dataclass * break out even more common code * break building pm report out of the run method * break the run trials loop out of the run method * format/lint * break building test report out of the run method in gravimetric run method * add the ui header to the build_report methods * move all of the dye prep into the dye prep method * improve LiquidTracker initialization * move the homing to execute_trials * refactor out common end script code * refactor out common get_tips * fixup things that broke during rebase * fix the photometric test target * remove evaporation measurement out of run for readablity * break out load scale for readablity * redo: Support resupplying tipracks during a 96ch increment tests * redo: Remove tips using them for finding liquid height * fix the tests * replace all print statements with ui * move the if-else out of the function to decrease the indent * make a base class config * fixup things that were lost in the rebase * construct the trial directory correctly * make sure 96 pipette doesn't collide during gravimetric tests * gracefully handle when the scale is beyond max * reimplement: Merge latest volumetric changes (#13100) * add stable flag to the blank gravimetric trials * Convert the default test volumes to the QC volumes * add default number of trials and correct volumes to match calculator sheet * make blank the default * update makefile to test the correct args for each tip * update increment test values * make single channle QC test work with all tips in one run * last few changes * fixup typing error * forgot a docstring for the linter * support the enviornmental sensor in the volumetric tests * make the asair prompt for port instead of auto finding * fix the tests
caila-marashaj
pushed a commit
that referenced
this pull request
Jul 26, 2023
* Revert "Merge latest volumetric changes (#13100)" This reverts commit 773b8a4. * Revert "fix(hardware-testing): Remove tips using them for finding liquid height (#13064)" This reverts commit cc0f509. * Revert "support resupplying tipracks during a 96ch increment test run (#13065)" This reverts commit 965f02d. * Pull out all of the common functions between gravimetric and photometric * move trial generation out of excecute * combine the load pipette logic * pull gantry speed out of the photometric config * refactor out load tipracks * remove unused method * fix issues from rebasing * fixups from rebase and formating * refactor out trial building from the photometric script * add additional test targets * pull out the common elements of trials into a seperate dataclass * break out even more common code * break building pm report out of the run method * break the run trials loop out of the run method * format/lint * break building test report out of the run method in gravimetric run method * add the ui header to the build_report methods * move all of the dye prep into the dye prep method * improve LiquidTracker initialization * move the homing to execute_trials * refactor out common end script code * refactor out common get_tips * fixup things that broke during rebase * fix the photometric test target * remove evaporation measurement out of run for readablity * break out load scale for readablity * redo: Support resupplying tipracks during a 96ch increment tests * redo: Remove tips using them for finding liquid height * fix the tests * replace all print statements with ui * move the if-else out of the function to decrease the indent * make a base class config * fixup things that were lost in the rebase * construct the trial directory correctly * make sure 96 pipette doesn't collide during gravimetric tests * gracefully handle when the scale is beyond max * reimplement: Merge latest volumetric changes (#13100) * add stable flag to the blank gravimetric trials * Convert the default test volumes to the QC volumes * add default number of trials and correct volumes to match calculator sheet * make blank the default * update makefile to test the correct args for each tip * update increment test values * make single channle QC test work with all tips in one run * last few changes * fixup typing error * forgot a docstring for the linter * support the enviornmental sensor in the volumetric tests * make the asair prompt for port instead of auto finding * fix the tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview
Test Plan
Changelog
Review requests
Risk assessment