refactor(api): redefine well geometry structure #16392
Conversation
| # A truncated circle is a square that is trimmed at the corners by a smaller circle | ||
| # that is concentric with the square. |
There was a problem hiding this comment.
I suggest centralizing documentation like this inside the JSON schema, and thinking of the Python bindings as dumb references to it. Basically just for single-source-of-truth reasons.
I would even go as far as to delete the pydantic.Field descriptions in labware_definition.py, though others have disagreed with me on that.
There was a problem hiding this comment.
Inline changes.
I also don't quite get the point of taking this data from a labware definition, to a pydantic model, and then out to a typed dict. What does the typed dict give us that the pydantic model doesn't? It kind of seems like it just makes us define everything twice.
ryanthecoder
force-pushed
the
EXEC-736-well-geometry-refactor
branch
from
2396daa
to
965ccb4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #16392 +/- ##
===========================================
+ Coverage 63.28% 81.14% +17.85%
===========================================
Files 300 106 -194
Lines 15786 3786 -12000
===========================================
- Hits 9990 3072 -6918
+ Misses 5796 714 -5082
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ryanthecoder
force-pushed
the
EXEC-736-well-geometry-refactor
branch
from
2aa5b9b
to
59e4130
Compare
There was a problem hiding this comment.
Shapes look great to me, thanks!
I haven't reviewed the processing part of this because it seems like @sfoster1 has already looked at it, but I'm happy to if you want more eyes.
| "properties": { | ||
| "shape": { | ||
| "type": "string", | ||
| "enum": ["squaredcone"] |
There was a problem hiding this comment.
Nit, doesn't block merging: camelCase squaredCone and roundedCuboid, if possible.
Unless these are matching a convention elsewhere in the labware definitions that I'm missing.
| @@ -350,7 +350,7 @@ class SquaredConeSegment(BaseModel): | |||
|
|
|||
|
|
|||
| """ | |||
| module filitedPyramidSquare(bottom_shape, diameter, width, length, height, steps) { | |||
| module filitedCuboidSquare(bottom_shape, diameter, width, length, height, steps) { | |||
There was a problem hiding this comment.
Nit for these OpenSCAD snippets, doesn't block merging: filleted instead of filited.
Overview
After discovering some new shapes and generally interacting with the new well geometry data structures, I think it would be better to reshape the geometry data a little bit. Rather than having each section of a well be represented by its top cross-section and top height, let's just represent a section in its entirety, with bottom and top cross-sections and bottom and top heights being present in every shape that is not a
SphericalSegment.Changelog
RoundedRectangleclassTruncatedCircleclassCircularFrustumandRectangularFrustumclassesfrustum_helpersand tests to use the new data structureTODO