-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: fix units for correct WellPlatePlan iteration #172
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #172 +/- ##
==========================================
- Coverage 98.28% 95.54% -2.74%
==========================================
Files 14 15 +1
Lines 874 1056 +182
==========================================
+ Hits 859 1009 +150
- Misses 15 47 +32 ☔ View full report in Codecov by Sentry. |
good one, i think i'm gonna play around with using pint here in another PR... should help us avoid this stuff a bit |
pos: List[Position] = [] | ||
for offset in offsets: | ||
if isinstance(offset, GridPosition): | ||
# invert y axis to use the cartesian coordinate system |
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 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.
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.
(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)
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.
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
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.
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.
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.
@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?
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.
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
- explicitly lays out all of the objects in the codebase that are in some way "coordinate-system aware"
- clarifies who/what defines that coordinate system
- 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.
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.
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
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'll merge this so you can keep moving forward, but this definitely needs further treatment 👍
I think we needed to fix the unit of few methods of the
WellPlatePlan
since thewell_points_plan
and the stage coordinates are expressed inµm
and theWellPlate
inmm
.