ResourceStack, Plate, Lid inheritance logic fix

[ericguan04]

fix: resource stack handling for Lid in LiquidHandler and PlateHolder.

Specifically, Lid will now be assigned to parent Plate if top item of ResourceStack is Plate. Change to carrier.py ensures that ResourceStack is inferred correctly (up to two levels of parent above, since Lid may be child of plate, or directly a child of ResourceStack).

Future stability improvements: Geometric Plate comptability check before assigining as child. If the plate will not slot along skirting, it should not register as a child.

[rickwierenga]

perhaps it makes more sense to make these different cases ((destination = ResourceStack, resource = Lid), (destination = Plate, resource = Lid)) into two 'top level' if statements?

or merge the (destination = ResourceStack, resource = Lid) case into the (destination=ResourceStack) cause above?

[ericguan04]

perhaps it makes more sense to make these different cases ((destination = ResourceStack, resource = Lid), (destination = Plate, resource = Lid)) into two 'top level' if statements?

or merge the (destination = ResourceStack, resource = Lid) case into the (destination=ResourceStack) cause above?

Hello, thank you for the suggestions! I noticed before that you left a comment for carrier.py but I can't see it here anymore. I thought the method you outlined there was very clean and a more catch all solution for finding the parent ResourceStack. For the liquid_handler.py, I can look into making the logic more clean via your suggestions above!

[rickwierenga]

i deleted the other comment because I realized i was wrong :)

(i made a comment suggesting to get the resource stack in the "parent-path" until the root. That is wrong because this method specifically exists to update the location of the first resource in the resource stack: if there are resources in between the given resource and the stack, it should probably be the location of the direct child of the resource that determines its location, not some [grand]child.)

[ericguan04]

Ah got it, that makes sense!

[BioCam]

Thank you for the awesome work!

I'm not sure what you mean by this:

Future stability improvements: Geometric Plate comptability check before assigining as child. If the plate will not slot along skirting, it should not register as a child.

@ericguan04, could you please elaborate? :)

[ericguan04]

Thank you for the awesome work!

I'm not sure what you mean by this:

Future stability improvements: Geometric Plate comptability check before assigining as child. If the plate will not slot along skirting, it should not register as a child.

@ericguan04, could you please elaborate? :)

Sure thing! My supervisor and I was just concerned that there is no check for incompatible plates+lids after implementing the logic above (automatically attaching a lid child to a plate if the plate is on the top level of the stack). On that note, we also thought about adding an argument that let's the user choose whether to automatically add the lid as a child to the plate on the top of the stack, since perhaps there may be a time where the user won't want the lid to be capped on. I hope that clarifies things!

[rickwierenga]

in this case, the parent is likely a plate. in that case, the location of the plate wrt the stack should be updated (and the plate should have a callback already registered) - not the lid.

[ericguan04]

Could you elaborate a little bit on what should be done with the plate / how the plate should be updated? For some more context, in the original codebase, I ran into an issue with move_lid where moving a lid to a plate on a stack would raise an exception because of the assert isinstance(resource_stack, ResourceStack) - 238. Thanks!

[rickwierenga]

it would be removing this case from the if statement

i think the problem with moving a lid to a plate on a stack is with liquid handler (since this callback should not be called in that case. if it is, there's a problem elsewhere)

[ericguan04]

Got it! I was also a little confused as to why this callback was called. I tried tracing out the code earlier today to see why but I had some trouble following it

[ericguan04]

The lid gets properly attached (lh.summary() shows this) with move_lid, but the code won't fully execute because of the assertion line

[rickwierenga]

then it seems that this callback is incorrectly called.

it should only be called when the first child of the ResourceStack changes

[rickwierenga]

incompatible plates+lids

great point! please PR in the future if you have time!

[BioCam]

incompatible plates+lids

great point! please PR in the future if you have time!

Depending on what you want PLR to do here, this might actually be a very difficult task:

If this is really needed, let's start with a definition set of what "incompatibility between plates and lids" means precisely in this context?

[maraxen]

incompatible plates+lids

great point! please PR in the future if you have time!

Depending on what you want PLR to do here, this might actually be a very difficult task:

If this is really needed, let's start with a definition set of what "incompatibility between plates and lids" means precisely in this context?

You're right, this isn't a small task. What we mean by incompatibility is an edge case where the lid should nest but doesn't.

Our current code, specifically the get_lid_location function, assumes lids will nest inside the plate:

if isinstance(destination, Plate):
  plate_location = destination.get_absolute_location()
  child_wrt_parent = destination.get_lid_location(
    lid.rotated(z=resource_rotation_wrt_destination_wrt_local)
  ).rotated(destination.get_absolute_rotation())
  to_location = plate_location + child_wrt_parent

And this calls:

def get_lid_location(self, lid: Lid) -> Coordinate:
  """Get location of the lid when assigned to the plate. Takes into account sinking and rotation."""
  return get_child_location(lid) + Coordinate(0, 0, self.get_size_z() - lid.nesting_z_height)

The issue arises when things like the lid's edge width or dimensions are slightly off. The result? The lid doesn't nest, and we end up with one of three outcomes:

'on_top': The lid just sits fully on top, completely above the plate.
'encompass': Much less likely, but the lid sits flush with the plate top, without adjusting for the nesting height.
Catastrophic tilting: The worst case, where the lid sits partially on top and is unstable.

Paths to Resolve This

I see a few ways we could tackle this:

1. Documentation: The simplest path. We just document that users are responsible for making sure their lids and plates are compatible and will stack correctly. We may also want to reference this issue so and more thoroughly address it only once it is actually brought up by more users.
2. Override Argument: We could add an argument, something like move_lid(independent: bool), which would treat the lid as its own object in the stack, so its Z-height registers properly. We could also add independent_behavior: Literal['on_top', 'encompass'] for more specific control.
3. Class-Based Check: A check like lid.__class_.__name__ in plate.__class__.__name__ to ensure the lid belongs to the plate class. While this is fairly simple, I think we would want to support actions like temporarily moving a lid onto a potentially different class of plate, so this may not be the most desirable.
4. Geometric Inference: This would be pretty complex to implement and thoroughly test, especially for an edge case, but it would be the most robust and general solution in the long run.

My Recommendation

Given the potentially undesirable behavior with option 3 and the complexity of implementing option 4, I recommend Option 1 (Documentation) for now, and maybe Option 2 (Override Argument) as a good next step.

What are your thoughts?

[rickwierenga]

anything i can do to help move this forward?

[ericguan04]

anything i can do to help move this forward?

@rickwierenga just added some additional documentation to the ResourceStack notebook. it is in a new pull request because I tried switching branches (accidently made this PR from my local main) but now the branches and PRs are out of sync - apologies for that. See PR #583

Documentation: The simplest path. We just document that users are responsible for making sure their lids and plates are compatible and will stack correctly. We may also want to reference this issue so and more thoroughly address it only once it is actually brought up by more users.

From @maraxen suggestions, I tried implementing solution 1 - please check the documentation I added. what i wrote is definitely a draft so do let me know what needs to be changed for consistency, clarity, etc.