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.

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

feat: few updates for WellPlatePlan #171

Merged
merged 5 commits into from
Jul 4, 2024

Conversation

fdrgsp
Copy link
Contributor

@fdrgsp fdrgsp commented Jul 4, 2024

@tlambert03 these are few updates I think might be useful.

  • add name to GridPosition + update Position
  • make well_size required
  • add docstrings

I also added a test to check the serialization because I was getting something weird when working on the HCSWizard widget. Basically if you create a WellPlatePlan from a dict, the well_points_plan is always Position() no matter what you pass and I don't yet understand why...

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

nice! small comments


rows: int
columns: int
well_spacing: Tuple[float, float] # (x, y)
well_size: Union[Tuple[float, float], None] = None # (x, y)
well_size: Tuple[float, float] # (width, height)
Copy link
Member

Choose a reason for hiding this comment

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

my reasoning for making this optional was that it's only necessary if you want to take a few images per well within some area. It's obviously pretty important in that case... but if you just wanted to take one image per well it's not necessary. I do guess in that case someone could just put in a fake number if they don't care about where the edge of the well is? (I do see that it makes life harder elsewhere if you can't rely on it)

fine with me to make it required though

Copy link

codecov bot commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.71%. Comparing base (1657db7) to head (abe759e).
Report is 11 commits behind head on main.

Files Patch % Lines
src/useq/_plate.py 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   98.28%   95.71%   -2.57%     
==========================================
  Files          14       15       +1     
  Lines         874     1050     +176     
==========================================
+ Hits          859     1005     +146     
- Misses         15       45      +30     

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

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

Feel free to merge when ready

@fdrgsp fdrgsp merged commit 10e861b into pymmcore-plus:main Jul 4, 2024
22 of 24 checks passed
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