Skip to content
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.

Sign up for GitHub

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

fix: fix units for correct WellPlatePlan iteration #172

Merged
merged 6 commits into from
Jul 5, 2024
Merged

fix: fix units for correct WellPlatePlan iteration #172

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
06db6c2
fix: use same unit in plot
fdrgsp Jul 5, 2024
23dd21c
fix: unit
fdrgsp Jul 5, 2024
b1b55ae
test: fix
fdrgsp Jul 5, 2024
7a86825
fix: image_positions
fdrgsp Jul 5, 2024
dd11d90
fix: update plot
fdrgsp Jul 5, 2024
abe4000
fix: fix comment
fdrgsp Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions src/useq/_plate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@
def all_well_positions(self) -> Sequence[Position]:
"""Return all wells (centers) as Position objects."""
return [
Position(x=x, y=y, name=name)
Position(x=x * 1000, y=y * 1000, name=name) # convert to µm
for (y, x), name in zip(
self.all_well_coordinates, self.all_well_names.reshape(-1)
)
Expand All @@ -338,7 +338,7 @@
def selected_well_positions(self) -> Sequence[Position]:
"""Return selected wells (centers) as Position objects."""
return [
Position(x=x, y=y, name=name)
Position(x=x * 1000, y=y * 1000, name=name) # convert to µm
for (y, x), name in zip(
self.selected_well_coordinates, self.selected_well_names
)
Expand All @@ -356,12 +356,21 @@
offsets: Iterable[Position | GridPosition] = (
[wpp] if isinstance(wpp, Position) else wpp
)
# TODO: note that all positions within the same well will currently have the
# same name. This could be improved by modifying `Position.__add__` and
# adding a `name` to GridPosition.
return [
well + offset for well in self.selected_well_positions for offset in offsets
]
pos: List[Position] = []
for offset in offsets:
if isinstance(offset, GridPosition):
# invert y axis to use the cartesian coordinate system
Copy link
Member

Choose a reason for hiding this comment

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

i recognize that this is necessary, but it definitely feels like too much local magic: works for your use case at the moment but makes assumptions that may not always be true. We can do this, but let's open a broader issue about coordinate systems and polarity.

Copy link
Member

Choose a reason for hiding this comment

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

(actually, this change would have been better for a different PR ... I'm 100% in support of the units changes, but this one gives me pause)

Copy link
Member

Choose a reason for hiding this comment

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

can you remove this, and then open a new issue. Please come up with a more general proposal for how someone can declare the polarity of their stage coordinate system. Think about all the places where this matters (I can imagine this comes up with GridRowColumn stuff as well), and then put the solution at a level where it can be accessed by everyone who needs it

Copy link
Contributor Author

@fdrgsp fdrgsp Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes 👍🏻 , what I tried to do within all the HCS logic is to keep the cartesian coordinates constant with y up, -y down, -x left and +x right. Also because we already used this coordinates system for the grid or random points plans for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tlambert03 don't you think it is better to just set a coordinate system that useq is using and is up to the user to convert the unit depending on their stage coordinate system? Otherwise we need to specify at least 4 different coordinate systems...

when you say "...come up with a more general proposal for how someone can declare the polarity of their stage coordinate system" are you thinking about something to set the sign of the 4 axis?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i'm generally observing that this is indeed a sticky point, that I'm still a little bit confused about exactly when and where these coordinate systems need to be declared, what they depend on (is it the stage? is it the plotting system? is it some transformation system? all of the above? etc...). And if I'm a bit confused, then I'm certain that the average user would probably be a bit confused...

And so I'd like to see it all given a more formal treatment, some documentation somewhere that

  1. explicitly lays out all of the objects in the codebase that are in some way "coordinate-system aware"
  2. clarifies who/what defines that coordinate system
  3. discusses how it should be customized in certain cases (for example, if my stage treats +X as to the left vs to the right... what should I do)?

don't you think it is better to just set a coordinate system that useq is using and is up to the user to convert the unit depending on their stage coordinate system?

once I understand the above a bit better, i think the answer to this question will probably be more obvious.

Copy link
Member

Choose a reason for hiding this comment

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

what I tried to do within all the HCS logic is to keep the cartesian coordinates constant with y up, -y down, -x left and +x right. Also because we already used this coordinates system for the grid or random points plans for example.

i'm glad you have this clear in your head :) but i don't, and a quick search through the code/documentation didn't reveal this. I also think it's still potentially unclear what "right" means. Does it mean "to the right in the image"? does it mean "the stage moves to the right when I look at it"? :) reference frames are confusing things and it's hard to be too clear about it all

Copy link
Member

Choose a reason for hiding this comment

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

i'll merge this so you can keep moving forward, but this definitely needs further treatment 👍

# (-y is up, +y is down, -x is left, +x is right)
offset = GridPosition(

Check warning on line 364 in src/useq/_plate.py

View check run for this annotation

Codecov / codecov/patch

src/useq/_plate.py#L364 

Added line #L364 was not covered by tests
x=offset.x,
y=-offset.y,
row=offset.row,
col=offset.col,
is_relative=offset.is_relative,
name=offset.name,
)
pos.extend(well + offset for well in self.selected_well_positions)
return pos

@property
def affine_transform(self) -> np.ndarray:
Expand Down Expand Up @@ -395,7 +404,8 @@
ax.axis("off")

# ################ draw outline of all wells ################
height, width = self.plate.well_size
height, width = self.plate.well_size # mm
height, width = height * 1000, width * 1000 # µm

Check warning on line 408 in src/useq/_plate.py

View check run for this annotation

Codecov / codecov/patch

src/useq/_plate.py#L407-L408 

Added lines #L407 - L408 were not covered by tests

kwargs = {}
offset_x, offset_y = 0.0, 0.0
Expand All @@ -420,13 +430,13 @@
)
ax.add_patch(sh)

# ################ plot image positions ################
################ plot image positions ################
w = h = None
if isinstance(self.well_points_plan, _PointsPlan):
w, h = self.well_points_plan.fov_width, self.well_points_plan.fov_height

for img_point in self.image_positions:
x, y = float(img_point.x), -float(img_point.y) # type: ignore[arg-type]
x, y = float(img_point.x), -float(img_point.y) # type: ignore[arg-type] # µm

Check warning on line 439 in src/useq/_plate.py

View check run for this annotation

Codecov / codecov/patch

src/useq/_plate.py#L439 

Added line #L439 was not covered by tests
if w and h:
ax.add_patch(
patches.Rectangle(
Expand Down
4 changes: 2 additions & 2 deletions tests/test_well_plate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ def test_plate_plan() -> None:
assert isinstance(pos0, useq.Position)
assert pos0 == pp[0]
assert pos0.name == "B1"
assert round(pos0.x, 2) == 500.78 # first row, but rotataed slightly
assert round(pos0.y, 2) == 208.97 # second column
assert round(pos0.x, 2) == 500784.4 # first row, but rotataed slightly, µm
assert round(pos0.y, 2) == 208965.75 # second column, µm

js = pp.model_dump_json()

Expand Down