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(labware-creator): export and import tiprack defs #7947

Merged
merged 5 commits into from
Jun 21, 2021

Conversation

IanLondon
Copy link
Contributor

@IanLondon IanLondon commented Jun 15, 2021

Overview

Closes #7696 and closes #7697

Changelog

  • Modifies the tiprack 300 fixture because it was internally inconsistent such that it could not be generated from createRegularLabware

Review requests

  • Code review, sanity check.
  • Smoke test by saving + loading + saving a tiprack def (or load a standard def then save).

Risk assessment

Low, as long as all tests are passing. This fixture is widely used in tests, but not in production code, so if all tests still pass we should be fine.

Notes

Eventually we could save this data elsewhere instead of in a PR, but, how I generated the 300 ul tiprack fixture def:

    const totalLiquidVolume = 300
    const commonWellProperties = {
      depth: 59.3,
      totalLiquidVolume,
    }
    const wellProperties: LabwareWellProperties = {
      ...commonWellProperties,
      shape: 'circular',
      diameter: 5.23,
    }

    const result = createRegularLabware({
      strict: false,
      metadata: {
        displayName: '300ul Tiprack FIXTURE',
        displayCategory: 'tipRack',
        displayVolumeUnits: 'µL',
        tags: [], // specifying tags is not yet supported
      },
      parameters: {
        format: '96Standard',
        isTiprack: true,
        tipLength: 123, // oops I edited this manually later, see below
        isMagneticModuleCompatible: false,
      },
      dimensions: {
        xDimension: 127.76,
        yDimension: 85.48,
        zDimension: 64.49,
      },
      brand: { brand: 'Fixture Brand' },
      version: 1,
      offset: {
        x: 14.38,
        y: 85.48 - 74.24,
        z: 64.49,
      },
      grid: {
        column: 12,
        row: 8,
      },
      spacing: {
        column: 23.38 - 14.38,
        row: 74.24 - 65.24,
      },
      well: wellProperties,
      group: {
        metadata: {},
      },
    })

Then, oops, I manually changed tipLength from 123 to 59.3

And manually changed the namespace to "fixture", and manually changed load name to fixture_tiprack_300_ul to match file name

@codecov
Copy link

codecov bot commented Jun 15, 2021

Codecov Report

Merging #7947 (8e0847b) into edge (77685a5) will increase coverage by 2.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #7947      +/-   ##
==========================================
+ Coverage   83.86%   85.92%   +2.06%     
==========================================
  Files         352      348       -4     
  Lines       21730    21235     -495     
==========================================
+ Hits        18223    18246      +23     
+ Misses       3507     2989     -518     
Impacted Files Coverage Δ
opentrons/protocol_engine/state/substore.py 91.66% <0.00%> (-8.34%) ⬇️
opentrons/protocol_engine/state/state_store.py 95.91% <0.00%> (-2.05%) ⬇️
opentrons/protocol_engine/state/labware.py 100.00% <0.00%> (ø)
opentrons/protocol_engine/state/geometry.py 100.00% <0.00%> (ø)
opentrons/tools/overnight_test.py
opentrons/tools/factory_test.py
opentrons/tools/z_stage_test.py
opentrons/tools/gantry_test.py
opentrons/protocol_engine/state/commands.py 100.00% <0.00%> (+10.52%) ⬆️
opentrons/tools/write_pipette_memory.py 56.36% <0.00%> (+22.40%) ⬆️

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 77685a5...8e0847b. Read the comment docs.

@IanLondon IanLondon marked this pull request as ready for review June 16, 2021 18:10
@IanLondon IanLondon requested a review from a team June 16, 2021 18:10
@IanLondon IanLondon requested review from a team as code owners June 16, 2021 18:10
@IanLondon IanLondon requested review from a team, b-cooper, Kadee80 and shlokamin and removed request for a team June 16, 2021 18:10
@@ -82,7 +82,7 @@ describe('DeckSetup', () => {
calBlock: mockTipLengthCalBlock,
})
expect(wrapper.text()).toContain(
'Clear the deck and place a full Opentrons GEB 300uL Tiprack and Calibration Block on'
'Clear the deck and place a full 300ul Tiprack FIXTURE and Calibration Block on'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be clearer if the fixture has "FIXTURE" in its name, instead of being called "Opentrons GEB" which isn't what this labware def is

@@ -114,7 +115,7 @@ export function labwareDefToFields(
shape.shape === 'rectangular' ? String(shape.yDimension) : null,

brand: def.brand.brand,
brandId: def.brand.brandId ? def.brand.brandId.join(',') : null, // comma-separated values
brandId: def.brand.brandId != null ? def.brand.brandId.join(',') : null, // comma-separated values
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just getting rid of some lint warnings about implicit falsey while I'm in here

@@ -2817,7 +2817,7 @@ Object {
"tiprack1Id": Object {
"def": Object {
"brand": Object {
"brand": "Opentrons",
"brand": "Fixture Brand",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

step-generation's makeContext fn is snapshot tested bc it's a huge fixture built up of other fixtures, including the tiprack def. So this is an expected change. All the tests still pass, which isn't a surprise bc they don't care about the geometry other than it being a 96-tip tiprack

@IanLondon IanLondon marked this pull request as draft June 16, 2021 18:21
@@ -91,7 +91,7 @@ export interface LabwareFields {
aluminumBlockType: string | null | undefined // eg, '24well' or '96well'
aluminumBlockChildType: string | null | undefined

handPlacedTipFit: string | null | undefined
handPlacedTipFit: 'snug' | 'loose' | null | undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

'snug' | 'loose' is similar to BooleanString = 'true' | 'false' here

@@ -79,7 +80,9 @@ export function labwareDefToFields(
tubeRackInsertLoadName: null,
aluminumBlockType: null,
aluminumBlockChildType: null,
handPlacedTipFit: null,

// We assume all tipracks are snug upon import
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@howisthisnamenottakenyet said:

Let’s not ask again. If a definition is being imported we can assume it was a snug fit when created.

@IanLondon IanLondon marked this pull request as ready for review June 16, 2021 19:02
Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

Everything looks good to me 🌵

@@ -10622,7 +10622,7 @@ exports[`LabwareList component renders 1`] = `
"dimensions": Object {
"xDimension": 127.76,
"yDimension": 85.48,
"zDimension": 64.48,
"zDimension": 64.49,
Copy link
Contributor

Choose a reason for hiding this comment

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

was this just some rounding error that snuck in? I'm not too worried about it, just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDK where the fixture's original 64.48 came from, but to re-generate it I copied opentrons_96_tiprack_300ul's 64.49 🤷

@IanLondon IanLondon merged commit a90e66d into edge Jun 21, 2021
@IanLondon IanLondon deleted the lc_fieldsToLabware-7696 branch June 21, 2021 18:20
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.

LC: load and save integration test case for custom tiprack Implement fieldsToLabware for tipracks
2 participants