Skip to content

Conversation

@rickwierenga
Copy link
Member

This PR refactors the ItemizedResource class (and its subclasses like Plate and TipRack) to store item ordering as an OrderedDict[str, str] mapping from identifier (e.g. “A1”) to child name (e.g. “plate_A1”) instead of a List[str]. This enables unambiguous conversion between indices and identifiers, resolving issues where get_item() and other methods could retrieve the wrong child due to reordered and/or extra children. The example that inspired this is a Lid as a child of Plate in front of Wells, messing up the well indexing. See #565.

Key Changes:

  • ItemizedResource._ordering is now an OrderedDict[str, str] mapping identifier to item name.
  • All usages of self._ordering that previously relied on list indexing have been updated to use the mapping correctly.
  • index_of_item() and get_child_identifier() have been updated to work with item names via _ordering.values(). No API change.
  • Constructors for Plate, TipRack, and NestedTipRack now accept an OrderedDict for ordering.

Why:

Previously, child retrieval methods relied on a flat list of identifiers without a stable mapping to actual child names. This was fragile in cases where there are other children (such as a Lid on a Plate). By tracking the mapping explicitly, we can safely reference children by both identifier and name without relying on implicit positional assumptions.

@rickwierenga rickwierenga requested a review from Copilot June 10, 2025 00:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors how ItemizedResource and its subclasses track item order by switching from a List[str] to an OrderedDict[str, str] mapping identifiers to child names, and updates all lookup and indexing methods to use this mapping.

  • Constructors in ItemizedResource, Plate, and TipRack now accept an OrderedDict for ordering instead of a list.
  • Methods like __getitem__, get_item, index_of_item, and get_child_identifier have been updated to work with the new _ordering mapping.
  • Imported OrderedDict where needed and adjusted assignment of _ordering in the base class initializer.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
pylabrobot/resources/tip_rack.py Changed ordering parameter type to OrderedDict in constructors (TipRack/NestedTipRack).
pylabrobot/resources/plate.py Updated ordering parameter type to OrderedDict in Plate constructor.
pylabrobot/resources/itemized_resource.py Switched _ordering from list to OrderedDict, updated slicing and lookup methods.
Comments suppressed due to low confidence (4)

pylabrobot/resources/tip_rack.py:124

  • The docstring for the ordering parameter in these constructors still describes a List[str]. Please update it to explain that it now expects an OrderedDict[str, str] mapping identifiers to child names.
ordering: Optional[OrderedDict[str, str]] = None,

pylabrobot/resources/plate.py:77

  • The docstring above this signature still refers to ordering as a list of identifiers. It should be updated to reflect that ordering is now an OrderedDict from identifier to child name.
ordering: Optional[OrderedDict[str, str]] = None,

pylabrobot/resources/itemized_resource.py:158

  • [nitpick] Converting self._ordering.keys() to a list and calling index() on every lookup may be inefficient for large resources. Consider caching the list of keys or maintaining a reverse mapping from identifier to index for faster lookups.
start = list(self._ordering.keys()).index(identifier)

pylabrobot/resources/itemized_resource.py:158

  • New behavior for identifier-based slicing in __getitem__ (and similar string‐based index lookups) should be covered by unit tests to ensure that slices using string identifiers return the expected items.
start = list(self._ordering.keys()).index(identifier)

@rickwierenga rickwierenga merged commit b42e4d6 into main Jun 10, 2025
6 checks passed
@rickwierenga rickwierenga deleted the fix-itemized-resource-get-item branch June 10, 2025 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants