refactor(api, shared-data): Refine locating features

[mjhuff]

Works towards EXEC-77

Overview

After finalizing our approach to locating features with @SyntaxColoring , we've made some changes into how we are defining a few top level properties and how we plan to implement LFs. LFs are informally fleshed out in this doc now. This PR serves to lay the groundwork before diving into adding each locating feature. It's definitely easiest to view this PR commit-by-commit, as it's mainly a lot of noise.

There's nothing too noteworthy here as future PRs that actually add each locating feature will clarify this work, however, the "footprint" extents, which represent the extents confined to a slot, are now considered a locating feature, slotFootprintAsChild. We don't do anything with this feature as of yet.

There are several scripts created and/or modified to migrate the code. These are in our work's scripts repo under a few branches: migrate_to_extents_v3, update_deck_defs, and update_module_defs. It's important to note that there is some duplication in definition fields now. It initially seemed like we could use pydantic's virtual properties to derive extents and some locating features, but alas, ModuleDefinition and DeckDefinitionV5 are TypedDicts, and it's quite a bit of effort to migrate these to pydantic, and more importantly, these wouldn't work with our typescript code anyway. I think the actual solution is to remove boundingBox and cornerOffsetFromSlot from related defs, see EXEC-1625.

89aeb4e adds some more dummy labware defs so we can properly test all planned locating features via the snapshot script as we add each new LF.

Test Plan and Hands on Testing

• There are no intentional functional changes here confirmed via snapshot (which I'll commit before merging). I am running the snapshot script primarily to serve as a baseline for schema 3 labware defs when we actually add locating features. We'll be able to see the impact of each locating feature on positioning as we add them. Note that some of the dummy labware will have signficantly different deck points than their real counterparts, because the dummy labware do not handle any sort of locating feature support yet (while the real ones still utilize COFS).
• Sufficiently covered by CI.

Changelog

• Moved extent footprint logic to a locating feature.
• Renamed locatingFeaturesAsParent to features.
• Removed locatingFeaturesAsChild.

Review requests

• I've marked slotFootprintAsChild as required for each labware. I think this is correct, but do others feel differently?
• For extents on module defs and deck defs, these are converted to the coordinate IV system, so they parallel labware def v3s. Are we ok with that?

Risk assessment

none

[codecov]

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 25.31%. Comparing base (b19784c) to head (5ffaa72).
Report is 81 commits behind head on edge.

Files with missing lines	Patch %	Lines
shared-data/js/helpers/labwareSchemaShims.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #18672      +/-   ##
==========================================
+ Coverage   24.12%   25.31%   +1.19%     
==========================================
  Files        3282     3284       +2     
  Lines      285236   285032     -204     
  Branches    28696    29361     +665     
==========================================
+ Hits        68808    72154    +3346     
+ Misses     216403   212857    -3546     
+ Partials       25       21       -4     

Flag	Coverage Δ
protocol-designer	19.54% <0.00%> (+0.29%)	⬆️
step-generation	5.38% <0.00%> (+0.13%)	⬆️

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

Files with missing lines	Coverage Δ
...ed-data/python/opentrons_shared_data/deck/types.py	100.00% <100.00%> (ø)
...pentrons_shared_data/labware/labware_definition.py	79.00% <100.00%> (+0.07%)	⬆️
...data/python/opentrons_shared_data/labware/types.py	100.00% <100.00%> (ø)
...-data/python/opentrons_shared_data/module/types.py	100.00% <100.00%> (ø)
shared-data/js/helpers/labwareSchemaShims.ts	32.63% <0.00%> (-8.43%)	⬇️

... and 251 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.

[mjhuff]

@SyntaxColoring , So one thing some of the new dummy labware migrations have shown is that some of the assumptions about footprint dimensions respective to extent totals are incorrect, and this is due to the fact that we don't factor in COFS for footprint calculations. We were just getting away with this because COFS on those definitions was always 0,0,0. I think we're correct in just deleting these tests, yeah?

[SyntaxColoring]

Thank you! Working my way through this. First batch of comments:

[sfoster1]

Love this approach. Couple requests

[sfoster1]

it seems bad to bake in this stuff with known-wrong numbers, might be pretty easy to forget to fix. would prefer to not have them or to gesture at having real values

[sfoster1]

wait, especially since we have actual numbers in shared-data

[mjhuff]

Oh wait, I totally thought this isn't a test case, whoops. Thanks.

[SyntaxColoring]

I'm still trying to grok whether/why this is necessary, but my knee-jerk reaction is that an addressable area ought not have extents.total because it muddies the waters. Cutout fixtures, the physical things, have extents.total, 3D bounding boxes that the robot needs to path around. What is the "3D bounding box" around an addressable area?

[mjhuff]

If an addressable area ought not have extents.total, then it ought not have a 3D boundingBox, which are defined in the schema as "The active area (both pipettes can reach) of this addressable area." I'm fine removing the extents from AAs, but we should probably then remove boundingBox from AAs in some later follow-up.

[SyntaxColoring]

I've thought about this some more and I think I can describe more clearly now why this was bothering me. Let me start over:

With our new feature system, we have a bunch of feature types that each define their dimensions in their own specific way. So slotFootprintAsChild has backLeftBottom/frontRightTop/z, and other features will have their own properties.

In the older addressable area system, things work differently. Each addressable area generically has a boundingBox, and then the areaType defines the interpretation of that bounding box.

What was bothering me was that a slotFootprintAsParent feature covers exactly what areaType: "slot" + boundingBox used to cover, so it doesn't make sense for them both to coexist (except as a temporary compromise while we port all our code).

boundingBox might look like it's a different thing that needs to be preserved, because it's 3D and it's "the active area (both pipettes can reach)." But those differences aren't actually worth preserving for deck slots. The older addressable area system needed those nuances for things like trashes, and because deck slots shared the same boundingBox schema as trashes, they incidentally applied to deck slots too.

[mjhuff]

Hmm, no, that doesn't sound right. Could you clarify what you mean by "we don't factor in COFS for footprint calculations"? Is that just a bug in the migration script?

Let's sync on this. Some COFS do have a positive value, which makes this test fail.

EDIT: After syncing, we've decided that any time there is a non-zero COFS value, we'll have to manually review the definition and determine slotFootPrintAsChild ourselves.

[SyntaxColoring]

Love it, thank you. Just a handful of minor things.