Add Grid Mode Spacing to change distance between layers #7597
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7597 +/- ##
==========================================
- Coverage 92.87% 92.84% -0.04%
==========================================
Files 629 629
Lines 58839 58898 +59
==========================================
+ Hits 54649 54685 +36
- Misses 4190 4213 +23 ☔ View full report in Codecov by Sentry. |
|
@TimMonko |
|
@psobolewskiPhD thanks for the review and suggestion -- very helpful! I'm guessing you are normally able to submit PRs to forks? I'll check through my settings, but I don't ever remember changing anything. |
|
A fork is just a repo with branches so should be able to open PRs on it. (Note: i can't suggest the change here in review because that part (in fact the whole file) isn't touched.) Edit: what's weird is when in the Open PR page I do compare across forks, I don't see @Czaki either, when I know I've made a PR to his fork before. So could be something else a-foot. |
…widgets via schema
|
@psobolewskiPhD @Czaki I made a pretty big change. It's stretching my knowledge a bit, so make sure I set up all the custom classes and inheritances correctly.
My goal was to make this extensible in the future, and propagate changes into other parts (like using for the grid_spacing widget button) and allow any future floats in the schema to specify step size. |
napari/components/viewer_model.py
Outdated
| # total spacing accounts for the distance between layers, results in 0 if not grid mode (grid_size = [1, 1] - 1) | ||
| total_spacing = np.multiply( | ||
| scene_size, self.grid.spacing * (np.array(grid_size) - 1) | ||
| ) | ||
| size = np.multiply(scene_size, grid_size) + total_spacing |
There was a problem hiding this comment.
Noted in the community meeting:
Currently if you have a non-square layer, you will get uneven spacing between the layers.
Probably best to have a consistent "border" zone computed from just one of the dimensions -- the smallest?
|
At the community meeting today, we discussed how the spacing was not the same in both the horizontal and vertical dimension, as shown with the top image, and an identical adjustment should be made. @Czaki and @psobolewskiPhD I think this is ready for review. I only minimally updated the tooltip in settings, I could add the equivalent info that shows in the Grid widget, but then think we should move all that to appearance settings? This now uses the average of the layer size to calculate, to produce symmetrical spacing. With a 16:9 aspect ratio (common for microscope cameras), I think a case can be made for any of the calculations. However, once the aspect ratio becomes even less symmetrical (like 32:9, as would be a 2x1 tiled image), I think the most intuitive starts to become the average difference. Regardless, if a user is working with an unusual aspect ratio, this spacing is modifiable. However, this does mean values of 1 won't move the 1x the extent anymore, but rather the average, again, I think this is fine because who would actually want their images that far apart? 16 x 9 Aspect RatioBefore (uneven), 0.1: This commit, Average, 0.1: 32 x 9 Aspect RatioMean, 0.1: Min, 0.1: Optical IllusionJust for fun, clearly an optical illusion ... the distance between the layers does not appear symmetrical, but I can assure you that it is! |
|
Folks, what is the next step on this PR? 1) Review and merge as is. or 2) Add more functionality and then review? |
review and merge |
|
I updated the doc string to reflect the averaging of the height and width and marked as ready to merge! |
Can you describe this a bit more? |
|
@TimMonko by default the whole PR description (the opening post). So we like those to be updated to reflect the final state of the PR. So some of those questions/issues that have been addressed can be removed. |
|
Question @psobolewskiPhD: I like adding a paragraph at the top of the first post with a description of the final PR (perhaps with a horizontal line underneath as the divider). Do napari folks typically delete historical data from the first post? |
|
@willingc i think it varies, but at the end of the day we like the commit message to give a description of what was merged. One thing I think is helpful is to indicate/remove any TODO/issues that have actually been done. They can give some future contributor the wrong picture. |
|
Updated PR description. Thanks for all the help! |
This is a good point, I do think many times we have deleted the earlier post, and it's potentially confusing, if one wants to revisit the whole conversation. Definitely something to discuss and document in our review guidelines. I would probably support moving to maintaining an "initial PR description" section under the edits that is considered immutable. |
# References and relevant issues Depends on napari/napari#7597 Part of #568 # Description Updates the copy in the viewer tutorial to mention the new spacing parameter. --------- Co-authored-by: TimMonko <47310455+TimMonko@users.noreply.github.com> Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: |
References and relevant issues
#7587 includes the desire for the ability to add spacing between layers in Grid mode. At the Feb 5/6 meeting, we discussed adding this ability, and frequent grid mode proponents @DragaDoncila and @psobolewskiPhD may be especially interested in this PR. This spacing especially helps quick export of figures, and no longer having to precisely crop panels 😄
Description
viewer.reset_view()to properly center cameraconfloatfrom pydantic that accepts step size. This is then used to allow a 0.05 step size value for the preferences widget. Type usable for future float widgets!Notes:
viewer.reset_view()is tested, which would be good to ensure camera centering is correct. I have validated by hand that it works AND it works properly withviewer.export_figure(), so does not break anything as far as I can tellconfloattypeThe preference setting jumps by 1.0 increments, I would prefer also to have the 0.05 step size as used by the GUI widget, but can't figure out how to do this (only validating, not stepping).. versionadded::to a docstring in the napari repo lol. It does seem to apply, but no one has previously used it. We will find out if docs build it nicely on release.I don't know if this applies in the Final Checklist - I don't see this in code docstrings, so is it just changed in napari/docs? I thought the api docs were auto created:If an API has been modified, I have added a.. versionadded::or.. versionchanged::directive to the appropriate docstring (For more information see
the Sphinx documentation).