fix: use cartesian coordinate system in WellPlatePlan (-x left, +x right, -y down and +y up) #175

fdrgsp · 2024-07-05T20:31:11Z

linked to #173

I removed all the change of sign we had in the WellPlatePlan code and only invert the y axis when we apply the transformation so that the y axis is -y down and +y up. This way we use the same cartesian coordinate system that we are already using in the _PointsPlans.

codecov · 2024-07-05T20:39:35Z

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.65%. Comparing base (c8996b7) to head (a5fc170).

Files	Patch %	Lines
src/useq/_plate.py	84.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #175      +/-   ##
==========================================
+ Coverage   95.54%   95.65%   +0.10%     
==========================================
  Files          15       15              
  Lines        1056     1058       +2     
==========================================
+ Hits         1009     1012       +3     
+ Misses         47       46       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tlambert03 · 2024-07-05T20:47:53Z

src/useq/_plate.py

-        return (transformed[:2].T).reshape(coords.shape)  # type: ignore[no-any-return]
+        transformed_coords = (transformed[:2].T).reshape(coords.shape)
+        # invert Y values
+        transformed_coords[:, 0] = -transformed_coords[:, 0]


probably better to do this in the affine transform itself, just invert the scaling factor in the y axis:

scaling[:2, :2] = np.diag([self.plate.well_spacing[0], -self.plate.well_spacing[1]])

But is it the same? The output won't be identical...

theoretically it should be? did you try and it wasn't?

yeah, it does work, but the example I gave above was xy, instead of yx... use this:

# we invert the Y axis here to go from linearly increasing "index coordinates" # to cartesian "plate" coordinates (where y position decreases with increasing index) scale_y, scale_x = self.plate.well_spacing scaling[:2, :2] = np.diag([-scale_y, scale_x])

yup, I tried that as well but then the test give a slightly different result and if you use a rotation and plot the plate it is rotating in the opposite direction...

Ok I think I was not convinced for another reason...I was a bit confused with the output numbers of the positions. But I think it was because we had to also consider the units of a1_center_xy. I think we can say that a1_center_xy is in µm since most likely the stage coords are in µm (and the well_points_plan too) and we covert it during translation.

yeah, it's confusing! Should we consider expressing well sizes in microns too just to keep everything consistent? Is this the only place in useq where we use mm?

Yes I believe is the only place we use mm...you're right, maybe it is worth using µm for everything at this point...

i know that's also problematic since all plates specs will be in mm... maybe skip that for now

then I think this is ready for review

src/useq/_plate.py

tlambert03 · 2024-07-06T00:17:55Z

please also fix a new scaling issue that we now have in random points:

                if value.max_width == np.inf:
                    kwargs["max_width"] = plate.well_size[0] - (value.fov_width or 0.1)
                if value.max_height == np.inf:
                    kwargs["max_height"] = plate.well_size[1] - (
                        value.fov_height or 0.1
                    )

the plate.well_sizes need to be scaled by 1000

src/useq/_plate.py

fdrgsp · 2024-07-06T01:14:52Z

@tlambert03 out of curiosity, if you run the test for the plate locally, does test_plate_plan_serialization fail for you?

tlambert03 · 2024-07-06T01:16:20Z

nope, and not on CI either obviously... whats your traceback

fdrgsp · 2024-07-06T01:19:04Z

nope, and not on CI either obviously... whats your traceback

the well_points_plan is always defaulting to Position()...

    def test_plate_plan_serialization() -> None:
        pp = useq.WellPlatePlan(
            plate=96,
            a1_center_xy=(500, 200),
            rotation=5,
            selected_wells=np.s_[1:5:2, :6:3],
            well_points_plan=useq.RandomPoints(num_points=10),
        )
        js = pp.model_dump_json()
        pp2 = useq.WellPlatePlan.model_validate_json(js)
>       assert pp2 == pp
E       AssertionError: assert WellPlatePlan(plate=WellPlate(rows=8, columns=12, well_spacing=(9.0, 9.0), well_size=(6.4, 6.4)), a1_center_xy=(500.0, 200.0), rotation=5.0, selected_wells=(slice(1, 5, 2), slice(None, 6, 3)), well_points_plan=Position()) == WellPlatePlan(plate=WellPlate(rows=8, columns=12, well_spacing=(9.0, 9.0), well_size=(6.4, 6.4)), a1_center_xy=(500.0, 200.0), rotation=5.0, selected_wells=(slice(1, 5, 2), slice(None, 6, 3)), well_points_plan=RandomPoints(num_points=10, max_width=6399.9, max_height=6399.9))
E
E         At index 0 diff: Position(name='B1') != Position(x=2010.7489791050302, y=-10177.428033516471, name='B1_0000')
E         Right contains 36 more items, first extra item: Position(x=1776.901041996079, y=-9937.020701044301, name='B1_0004')
E
E         Full diff:
E         - WellPlatePlan(plate=WellPlate(rows=8, columns=12, well_spacing=(9.0, 9.0), well_size=(6.4, 6.4)), a1_center_xy=(500.0, 200.0), rotation=5.0, selected_wells=(slice(1, 5, 2), slice(None, 6, 3)), well_points_plan=RandomPoints(num_points=10, max_width=6399.9, max_height=6399.9))
E         ?                                                                                                                                                                                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E         + WellPlatePlan(plate=WellPlate(rows=8, columns=12, well_spacing=(9.0, 9.0), well_size=(6.4, 6.4)), a1_center_xy=(500.0, 200.0), rotation=5.0, selected_wells=(slice(1, 5, 2), slice(None, 6, 3)), well_points_plan=Position())
E         ?                                                                                                                                                                                                                   ^^^^^^^^^^^

tlambert03 · 2024-07-06T01:23:58Z

the well_points_plan is always defaulting to Position()...

strange... not sure. what's in your env?
it must be environment, since it's working on CI, but I wonder what it could be.

src/useq/_plate.py

fdrgsp · 2024-07-06T01:26:37Z

the well_points_plan is always defaulting to Position()...

strange... not sure. what's in your env? it must be environment, since it's working on CI, but I wonder what it could be.

ok fixed it, not sure what was the conflict...

tlambert03

good stuff, thanks

fdrgsp added 3 commits July 5, 2024 14:45

fix: update coords system

607fc6b

fix: return pos per well

0ed6387

test: add pos order test

f94a1b0

fdrgsp mentioned this pull request Jul 5, 2024

TODO: Clarify coordinate systems #173

Open

Merge branch 'main' into fix_sign

674ef8a

tlambert03 reviewed Jul 5, 2024

View reviewed changes

src/useq/_plate.py Show resolved Hide resolved

fdrgsp added 3 commits July 5, 2024 20:25

fix: _validate_well_points_plan unit

12e92cf

fix: invert y in affine + fix a1_center_xy unit for affine

d4bc4c1

test: fix

162c71d

tlambert03 reviewed Jul 6, 2024

View reviewed changes

src/useq/_plate.py Outdated Show resolved Hide resolved

fix: revert

2aada09

tlambert03 reviewed Jul 6, 2024

View reviewed changes

src/useq/_plate.py Outdated Show resolved Hide resolved

Update src/useq/_plate.py

a5fc170

update docs

26e1bad

tlambert03 approved these changes Jul 6, 2024

View reviewed changes

tlambert03 merged commit 49be9ad into pymmcore-plus:main Jul 6, 2024
20 checks passed

fdrgsp deleted the fix_sign branch July 6, 2024 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use cartesian coordinate system in WellPlatePlan (-x left, +x right, -y down and +y up) #175

fix: use cartesian coordinate system in WellPlatePlan (-x left, +x right, -y down and +y up) #175

fdrgsp commented Jul 5, 2024

codecov bot commented Jul 5, 2024 •

edited

Loading

tlambert03 Jul 5, 2024

fdrgsp Jul 5, 2024

tlambert03 Jul 5, 2024

tlambert03 Jul 6, 2024

fdrgsp Jul 6, 2024

fdrgsp Jul 6, 2024

tlambert03 Jul 6, 2024

fdrgsp Jul 6, 2024

tlambert03 Jul 6, 2024

fdrgsp Jul 6, 2024

tlambert03 commented Jul 6, 2024

fdrgsp commented Jul 6, 2024

tlambert03 commented Jul 6, 2024

fdrgsp commented Jul 6, 2024

tlambert03 commented Jul 6, 2024

fdrgsp commented Jul 6, 2024

tlambert03 left a comment

fix: use cartesian coordinate system in WellPlatePlan (-x left, +x right, -y down and +y up) #175

fix: use cartesian coordinate system in WellPlatePlan (-x left, +x right, -y down and +y up) #175

Conversation

fdrgsp commented Jul 5, 2024

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlambert03 commented Jul 6, 2024

fdrgsp commented Jul 6, 2024

tlambert03 commented Jul 6, 2024

fdrgsp commented Jul 6, 2024

tlambert03 commented Jul 6, 2024

fdrgsp commented Jul 6, 2024

tlambert03 left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 5, 2024 •

edited

Loading