-
Notifications
You must be signed in to change notification settings - Fork 122
Fix issue with liquid tracking in aspirate96, dispense96, and enabling/disabling volume tracking #565
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
Conversation
…disable tracking functions
pylabrobot/resources/plate.py
Outdated
| for well in self.get_all_items(): | ||
| well.tracker.disable() | ||
| # get_all_items() returns all items, including the lid | ||
| # ignore the lid since it does not have a tracker | ||
| if not isinstance(well, Lid): | ||
| well.tracker.disable() | ||
|
|
||
| def enable_volume_trackers(self) -> None: | ||
| """Enable volume tracking for all wells in the plate.""" | ||
|
|
||
| for well in self.get_all_items(): | ||
| well.tracker.enable() | ||
| # get_all_items() returns all items, including the lid | ||
| # ignore the lid since it does not have a tracker | ||
| if not isinstance(well, Lid): | ||
| well.tracker.enable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_all_items should only return the items. If it doesn't (I can see how that would be the case, get_item calls .children[identifier]), there is an issue with ItemizedResource. In this case the root issue should be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just committed changes to ItemizedResource and reverted plate.py back to normal - let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is more so that the get_item method will indiscriminately get a child at a specific index. I am making a separate PR to fix this issue by saving the ordering<>name conversion, that we can use here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…into tracker_fix
3773c12 to
4b0070b
Compare
| for channel, well in zip(self.head96.values(), containers): | ||
| # even if the volume tracker is disabled, a liquid (None, volume) is added to the list | ||
| # during the aspiration command | ||
| liquids = channel.get_tip().tracker.remove_liquid(volume=volume) | ||
| reversed_liquids = list(reversed(liquids)) | ||
| all_liquids.append(reversed_liquids) | ||
| for well, channel in zip(containers, self.head96.values()): | ||
| # check if volume tracking is disabled | ||
| if well.tracker.is_disabled or not does_volume_tracking(): | ||
| reversed_liquids = [(None, volume)] | ||
| all_liquids.append(reversed_liquids) | ||
| else: | ||
| liquids = channel.get_tip().tracker.remove_liquid(volume=volume) | ||
| reversed_liquids = list(reversed(liquids)) | ||
| all_liquids.append(reversed_liquids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
liquids (or None if volume tracking is disabled) should have been added in aspiration. That means we can still rack all_liquids. This is needed to make the MultiHeadDispensePlate object.
we should keep the change where liquids are only added to the well tracker if liquid tracking is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason it's needed in MultiHeadDispensePlate is backends can use this information to change liquid handling parameters based on the type of liquid. None liquids mean unknown.
| else: | ||
| # tracker is enabled: update tracker liquid history | ||
| liquids = well.tracker.remove_liquid(volume=volume) # type: ignore | ||
| all_liquids.append(liquids) | ||
|
|
||
| for liquid, vol in reversed(liquids): | ||
| channel.get_tip().tracker.add_liquid(liquid=liquid, volume=vol) | ||
| for liquid, vol in reversed(liquids): | ||
| channel.get_tip().tracker.add_liquid(liquid=liquid, volume=vol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the liquids are computed in either case (volume tracking enabled or not). I don't understand why they are only added in the else case. They should actually be added to the channel head tracker in both cases since it is information we need on dispense (see other comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah got it, i didn't notice the distinction between well.tracker and channel.get_tip().tracker and assumed that they should all be controlled with the volume tracking condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, that's confusing.
container tracking is only when volume tracking is enabled globally (false by default) and for that container (true by default). for the channels we always need to track it to prevent over-aspiration / making sure tips are empty before putting them back in the rack
| for liquid, vol in reversed_liquids: | ||
| well.tracker.add_liquid(liquid=liquid, volume=vol) | ||
| for liquid, vol in reversed_liquids: | ||
| well.tracker.add_liquid(liquid=liquid, volume=vol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was likely part of the actual issue: the liquids weren't added to each well (just the last one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is dispense96 supposed to not have a tracker condition (if well.tracker.is_disabled or not does_volume_tracking(): ...)? in the original codebase, there was one for isinstance(resource, Container), but not for isinstance(resource, Plate). this is what i thought caused the issue since turning volume tracking off would prevent aspirate96 from tracking liquids, but dispense96 will do so regardless, causing the assert errors, but do let me know if i am misunderstanding something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should have that condition! for putting liquid into wells. that part is conditional on volume tracking being enabled
but or channels we always need it :)
this is what i thought caused the issue since turning volume tracking off would prevent aspirate96 from tracking liquids
in this case, asp96 would actually still track liquids.
the problem in this line (and i think the root cause of the issue you describe in the PR message) is that liquid doesn't get added to well on dispense. the code you change in these 2 highlighted lines was obviously wrong before
|
if you could provide a python snippet highlighting the issue with i wrote comments for parts of the code that i think were wrongly/correctly changed. I think the actual fix to the issue you describe above is just a two line indentation change. (I might be missing something though) |
Running on most recent upstream/main codebase:
|
|
thanks for the code. I reverted the change of adding only adding liquid to channel trackers when volume tracking is enabled (we should always track liquid in the channels to prevent overaspiration and dropping tips with liquid, unless overridden). I also reverted the change of not removing liquid from channel tip trackers when volume tracking is disabled. the result is now just adding liquid back to the wells when volume tracking is enabled. we weren't doing this before. |
|
thanks for flagging & fixing this! (& doing a little bit more ;)) |
Liquid tracking issue: The liquid tracking mechanism for aspiration and dispensing operations is not functioning as expected.
Through testing, it seems like
aspirate96does not actually reduce the liquid level in the plate after aspiration. This leads to inconsistencies in the tracked liquid volume and causes errors when subsequent dispensing operations are performed, as the system believes there is more liquid available than there actually is.Example: Assuming a plate with 360 ul volume limit, following this procedure:
Looking through the code, it turns out that aspirate96 had some tracking code outside the tracking conditional, and dispense96 has no tracking conditional at all. I believe this is the cause for the mismatch and subsequently the errors. After implementing the changes below, the code works as intended.
As a side note: If liquid tracking is enabled, it is now necessary to use
.set_well_liquids()prior or else there will be no liquid to aspirate (wasn't the case before). I personally believe that this change makes more sense if tracking liquid is a main concern. This does cause some test cases to fail, and it's perhaps because that.set_well_liquids()was not used.Additionally, enabling and disabling volume had some issues. First, for the aspirate96 and dispense96 functions, I removed the second conditional
...or not does_volume_tracking()since it makes it much harder to turn off volume tracking (just using.disable_volume_trackers()would be enough). I also added a condition to ignore lids for the enable/disable volume tracking commands.I did a good amount of testing with aspirating and dispensing, but I am not too sure about any downstream affects for other functions.