Skip to content

Conversation

@ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented Apr 23, 2025

Overview

Previously, the range displayed in FlowRateField was pulled directly from the pipette liquid definition. This value was specific only to the pipette + tip combination. In reality, this max flow rate is a function of the target volume, and correction volume for that aspiration/dispense, as well as the pipette + tip accuracy function and max plunger speed. This PR mirrors the hardware controller's logic for determining the specific max allowable flow rate for the step.

Closes AUTH-1727

Test Plan and Hands on Testing

This is kind of difficult to test! I am following the logic as outlined by @andySigler in the hardware controller.

Please review code logic, and smoke test by trying out a variety of combinations of volumes + pipettes + tips + liquid classes in transfer and mix fields, and verify that the flow rate does not exceed the range on the flow rate field.

Changelog

  • overhaul logic in FlowRateField for determining max flow rate to show in UI
  • provide formData to FlowRateField instances

Review requests

see test plan

Risk assessment

low. Strictly a UI update. Even if the volume is out of range, it is non-blocking for saving the step

Previously, the range displayed in `FlowRateField` was pulled directly from the pipette liquid definition. This value was specific only to the pipette + tip combination. In reality, this max flow rate is a function of the target volume, and correction volume for that aspiration/dispense, as well as the pipette + tip accuracy function and max plunger speed. This PR mirrors the hardware controller's logic for determining the specific max allowable flow rate for the step.

Closes AUTH-1727
@ncdiehl11 ncdiehl11 self-assigned this Apr 23, 2025
@ncdiehl11 ncdiehl11 requested review from jerader and koji April 23, 2025 19:59
@ncdiehl11 ncdiehl11 marked this pull request as ready for review April 23, 2025 19:59
@ncdiehl11 ncdiehl11 requested a review from a team as a code owner April 23, 2025 19:59
@ncdiehl11 ncdiehl11 removed the request for review from a team April 23, 2025 20:00
@codecov
Copy link

codecov bot commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 36.48649% with 94 lines in your changes missing coverage. Please review.

Project coverage is 23.22%. Comparing base (a74a574) to head (b173ffc).
Report is 2 commits behind head on edge.

Files with missing lines Patch % Lines
...ocolSteps/StepForm/PipetteFields/FlowRateField.tsx 0.00% 86 Missing ⚠️
...ols/MoveLiquidTools/SecondStepsMoveLiquidTools.tsx 0.00% 3 Missing ⚠️
...ocolSteps/StepForm/PipetteFields/DisposalField.tsx 0.00% 2 Missing ⚠️
...StepForm/StepTools/MixTools/SecondStepMixTools.tsx 0.00% 2 Missing ⚠️
...gner/ProtocolSteps/StepForm/PipetteFields/utils.ts 98.14% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #18165      +/-   ##
==========================================
- Coverage   23.76%   23.22%   -0.55%     
==========================================
  Files        3038     3038              
  Lines      253513   259921    +6408     
  Branches    23604    25665    +2061     
==========================================
+ Hits        60252    60368     +116     
- Misses     193246   199538    +6292     
  Partials       15       15              
Flag Coverage Δ
protocol-designer 19.00% <40.29%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...r/src/steplist/formLevel/handleFormChange/utils.ts 16.59% <100.00%> (-0.63%) ⬇️
...gner/ProtocolSteps/StepForm/PipetteFields/utils.ts 27.91% <98.14%> (+25.62%) ⬆️
...ocolSteps/StepForm/PipetteFields/DisposalField.tsx 2.10% <0.00%> (-0.10%) ⬇️
...StepForm/StepTools/MixTools/SecondStepMixTools.tsx 0.75% <0.00%> (-0.02%) ⬇️
...ols/MoveLiquidTools/SecondStepsMoveLiquidTools.tsx 0.53% <0.00%> (-0.07%) ⬇️
...ocolSteps/StepForm/PipetteFields/FlowRateField.tsx 1.08% <0.00%> (-1.22%) ⬇️

... and 754 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

96: MAX_PLUNGER_SPEED_FLEX_HIGH_THROUGHPUT_MM_PER_S,
}

const _getPipetteAccuracyUlPerMm = (
Copy link
Collaborator

@jerader jerader Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you separate out these 2 utils into a utils file and then add tests for getMaxUiFlowRate?

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit hard to follow the logic but would be great if you can add some test cases with various examples

@ncdiehl11
Copy link
Collaborator Author

a bit hard to follow the logic but would be great if you can add some test cases with various examples

it is definitely not straightforward— I copied this logic straight from what Andy sent from HW controller. I also think adding tests would be helpful

@ncdiehl11 ncdiehl11 requested a review from jerader April 24, 2025 21:06
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for creating the util and test to get more accurate max flow rates! left some clean up comments

@ncdiehl11 ncdiehl11 merged commit e9b109f into edge Apr 25, 2025
16 checks passed
@ncdiehl11 ncdiehl11 deleted the pd_fix-uiMaxFlowRate branch April 25, 2025 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants