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

[enh] add an add_plane convenience method to ClippingPlaneList #6921

Merged
merged 8 commits into from
Jul 22, 2024

Conversation

psobolewskiPhD
Copy link
Member

References and relevant issues

There is some discussion about this here
https://napari.zulipchat.com/#narrow/stream/212875-general/topic/clipping.20planes.20and.20slicing.20planes

Description

Prior to this PR, if a user wanted to use the experimental clipping planes feature, they had to do the following in the console or script:

from napari.layers.utils.plane import ClippingPlane
cplane = ClippingPlane()
viewer.layers[-1].experimental_clipping_planes.append(cplane)

In this PR, I add a convenience method to ClippingPlaneList called add_plane that will create a ClippingPlane with args/kwargs and then append it to the list, this way it's easier for users to get at this great feature! I also add a test for the new method.

@github-actions github-actions bot added tests Something related to our tests and removed enhancement labels May 19, 2024
@psobolewskiPhD psobolewskiPhD added this to the 0.5.0 milestone May 19, 2024
@psobolewskiPhD psobolewskiPhD changed the title [enh] add an add_plane convenience method to ClippingPlaneList [enh] add an add_plane convenience method to ClippingPlaneList May 19, 2024
@psobolewskiPhD
Copy link
Member Author

I'm not sure how def add_plane(self, *args, **kwargs) -> None: should be properly annotated. My google-fu was weak? So I just added the type: ignore, advice welcome.

Copy link

codecov bot commented May 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.86%. Comparing base (5a42fd9) to head (22e57df).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6921      +/-   ##
==========================================
- Coverage   92.96%   92.86%   -0.10%     
==========================================
  Files         618      618              
  Lines       56525    56539      +14     
==========================================
- Hits        52547    52505      -42     
- Misses       3978     4034      +56     

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

Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

Seems pretty straight forward to me, and it's clearly marked experimental so we don't need to stress about getting the API right. No clue about typing :/

@psobolewskiPhD
Copy link
Member Author

I think if I dropped the ->None: then mypy would also skip it? So maybe that's better?

@brisvag
Copy link
Contributor

brisvag commented May 20, 2024

yup, better untyped than ignored!

@psobolewskiPhD
Copy link
Member Author

Boo, dropping the None didn't work:
https://github.com/napari/napari/actions/runs/9165440649/job/25198887475?pr=6921#step:5:16

@jni jni modified the milestones: 0.5.0, 0.5.1 Jul 8, 2024
@jni
Copy link
Member

jni commented Jul 8, 2024

@psobolewskiPhD if you can figure out typing I think this could (still!) go into 0.5.0! Maybe @Czaki has some some advice...

@jni
Copy link
Member

jni commented Jul 10, 2024

@psobolewskiPhD the other thing I would do here is maybe add/update a gallery example.

@jni jni modified the milestones: 0.5.0, 0.5.1 Jul 10, 2024
napari/layers/utils/plane.py Outdated Show resolved Hide resolved
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
@jni
Copy link
Member

jni commented Jul 10, 2024

typing is still unhappy @Czaki 😢

@Czaki
Copy link
Collaborator

Czaki commented Jul 10, 2024

@jni fixed

@jni
Copy link
Member

jni commented Jul 11, 2024

Amazing. 😂

@psobolewskiPhD
Copy link
Member Author

Thanks @Czaki ❤️

@brisvag brisvag added the ready to merge Last chance for comments! Will be merged in ~24h label Jul 18, 2024
@jni jni merged commit d026ac0 into napari:main Jul 22, 2024
41 checks passed
@github-actions github-actions bot removed the ready to merge Last chance for comments! Will be merged in ~24h label Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants