Skip to content
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(shared-data): correct existing labware defs engageHeight #5261

Merged
merged 3 commits into from
Mar 23, 2020

Conversation

IanLondon
Copy link
Contributor

@IanLondon IanLondon commented Mar 20, 2020

overview

Closes #5226

changelog

  • existing 3 labware defs should be output in real mm from labware bottom

review requests

  • The 3 labware that use engageHeight should have their PD suggested defaults equal to ((original_number_in_def * 2) - 4) / 2. For GEN2 magnetic module, the final / 2 shouldn't happen, for GEN1, it should.

For the 1 recommended magnetic module labware, for nest_96_wellplate_100ul_pcr_full_skirt, both default value and "Recommended: __" caption should be:

  • GEN2: 6 real mm
  • GEN1: 12 short mm

FYI, the math is:

20 short mm from def for nest_96_wellplate_100ul_pcr_full_skirt
= 10 real mm from switch
Do -4 real mm to translate "from switch" to "from labware bottom"
= 6 real mm

GEN2: 6 real mm
GEN1: 12 short mm (6*2)

risk assessment

Shouldn't interfere with anything outside on engageHeight use in PD. But it WILL mess up creating engage magnet steps in PD with default values until we do #5230, because the min/max values we have aren't right; default values won't be valid until we fix the range. However this issue is blocking #5230 too.

@IanLondon IanLondon added ready for review protocol designer Affects the `protocol-designer` project labels Mar 20, 2020
@IanLondon IanLondon requested a review from a team as a code owner March 20, 2020 21:03
@@ -55,9 +55,6 @@ export const MODULE_MODELS: Array<ModuleModel> = [
...THERMOCYCLER_MODULE_MODELS,
]

// offset added to parameters.agneticModuleEngageHeight for displaying reccomended height in PD
export const ENGAGE_HEIGHT_OFFSET = -4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this from constants b/c nobody aside from getLabwareDefaultEngageHeight should use it, as far as I can figure. I think wanting the -4 would be a sign you're not going through the getter and probably you should use the getter...?

@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #5261 into edge will increase coverage by 0.72%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #5261      +/-   ##
==========================================
+ Coverage   61.44%   62.16%   +0.72%     
==========================================
  Files        1043     1043              
  Lines       29623    30363     +740     
==========================================
+ Hits        18201    18876     +675     
- Misses      11422    11487      +65
Impacted Files Coverage Δ
shared-data/js/constants.js 100% <ø> (ø) ⬆️
protocol-designer/src/ui/modules/selectors.js 33.33% <0%> (-5.38%) ⬇️
shared-data/js/getLabware.js 33.33% <40%> (+3.33%) ⬆️
protocol-designer/src/ui/steps/actions/actions.js 80% <0%> (-2.5%) ⬇️
opentrons/drivers/types.py 100% <0%> (ø) ⬆️
opentrons/hardware_control/api.py 90.07% <0%> (+2.42%) ⬆️
opentrons/drivers/smoothie_drivers/driver_3_0.py 80.83% <0%> (+5.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d28d38f...d5f19ce. Read the comment docs.

Copy link
Contributor

@Kadee80 Kadee80 left a comment

Choose a reason for hiding this comment

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

❄️

  • previous default value: 16
  • new GEN1 value: 18
  • new GEN2 value: 36

Approving assuming these are the new ranges/numbers!

labwareDef.namespace === OPENTRONS_LABWARE_NAMESPACE &&
_SHORT_MM_LABWARE_DEF_LOADNAMES.includes(labwareDef.parameters.loadName)
) {
const rawEngageHeight: ?number =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this part be moved out of the if block since it's repeated on line 83?

@Kadee80 Kadee80 self-requested a review March 23, 2020 19:18
@Kadee80
Copy link
Contributor

Kadee80 commented Mar 23, 2020

Dismissing my approval since i think there is some confusion of real and short mm numbers here and in ranges and what the UI should accept. i think we talked about getting rid of the units all together?

Copy link
Contributor

@Kadee80 Kadee80 left a comment

Choose a reason for hiding this comment

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

🐅
makes sense now!

@IanLondon IanLondon merged commit 767054d into edge Mar 23, 2020
@IanLondon IanLondon deleted the pd_getMagneticModuleEngageHeight-5226 branch March 23, 2020 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol designer Affects the `protocol-designer` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PD + shared-Data: patch getLabwareDefaultEngageHeight getter
3 participants