Add optional fixed `length` parameter to scale bar #7167

jo-mueller · 2024-08-07T20:23:34Z

Description

This PR adds a mew parameter length to the scale bar that allows to set the the scale to a fixed width in physical units which will grow or shrink together with the zoom of the canvas.

Changes

ScaleBarOverlay class
- Add length parameter with default value None
VispyScaleBarOverlay class
- Add handling for length parameter
- Update _on_zoom_change method to use fixed_width if not None
- Add _on_length_change method

Checklist

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to docstrings and documentation
(open a PR on the docs repository (https://github.com/napari/docs) if relevant!)
I have added tests that prove my fix is effective or that my feature works

Tests
- Add test for length parameter in VispyScaleBarOverlay class

…erlay` classes * **ScaleBarOverlay class** - Add `fixed_width` parameter with default value `None` * **VispyScaleBarOverlay class** - Add handling for `fixed_width` parameter - Update `_on_zoom_change` method to use `fixed_width` if not `None` - Add `_on_fixed_width_change` method * **Examples** - Add example usage of `viewer.scale_bar.fixed_width` in `examples/scale_bar.py` * **Tests** - Add test for `fixed_width` parameter in `VispyScaleBarOverlay` class - Add test for `fixed_width` parameter in `ScaleBarOverlay` class

codecov · 2024-08-07T21:09:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.92%. Comparing base (9a9e7e8) to head (0309623).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7167      +/-   ##
==========================================
- Coverage   93.01%   92.92%   -0.10%     
==========================================
  Files         619      620       +1     
  Lines       56757    56777      +20     
==========================================
- Hits        52792    52758      -34     
- Misses       3965     4019      +54

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

8000

brisvag

Love this! It's great to see how relatively small the change needed was, and how little help you needed to do this! Great work 💪

I left a few comments, none of them big issues.

The main thing I'm unsure about is the name width, which to me is a bit strange. Maybe this is typical nomenclature and I'm unaware, but to me width means the other direction ("thickness" of the line). So I'd rather call this length. Otherwise, size for something more generic. I feel like @psobolewskiPhD will have opinions on this :)

examples/scale_bar.py

napari/_vispy/_tests/test_vispy_scale_bar_visual.py

napari/_vispy/overlays/scale_bar.py

for more information, see https://pre-commit.ci

jo-mueller · 2024-08-08T08:41:11Z

Thanks @brisvag for the quick review :)

and how little help you needed to do this!

I must admit that the github workspace helps big time in getting started to find the right places for modifications :)

As for the naming conventions, I went with fixed_width because I could imagine that people may want to also control the thickness of the scale bar somehow, which would then have to be controlled by a parameter that can be easily distinguished from the width. That being said - now that I write it - the two parameters could be called length and thickness and that would probably be verbose enough to tell the average user what to do with them.

On the GUI for scale bar control - is there already an issue for this?

brisvag · 2024-08-08T08:43:03Z

That being said - now that I write it - the two parameters could be called length and thickness and that would probably be verbose enough to tell the average user what to do with them.

I find this more intuitive, yeah. No risk of confusing them, unlike "width".

One the GUI for scale bar control - is there already an issue for this?

I don't think so! Feel free to create and link to the other one!

napari/_vispy/_tests/test_vispy_scale_bar_visual.py

Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>

for more information, see https://pre-commit.ci

brisvag

Looking good to me :) Let's see what CI thinks about it now.

napari/components/overlays/scale_bar.py

Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>

napari/components/overlays/scale_bar.py

napari/components/_tests/test_scale_bar.py

melonora

Great work @jo-mueller ! Thanks for picking this up

Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>

Co-Authored-By: Lorenzo Gaifas <brisvag@gmail.com>

brisvag · 2024-08-08T12:36:37Z

Wrong line :P it's the self._target_length that needed changing!

Co-Authored-By: Lorenzo Gaifas <brisvag@gmail.com>

jo-mueller · 2024-08-08T12:54:33Z

I'm dumb 🙈 thanks for the fix(es) :)

brisvag · 2024-08-08T13:06:48Z

It did look very similar :P It's annoying cause when a code line is far enough from the changes in the PR, github doesn't allow to make suggestions on it, so you have to resort to linking like above ^^'

I think we might be done 💪

brisvag · 2024-08-08T13:59:53Z

@jo-mueller since the description will become the commit message, could you update it so it reflects the current status?

jo-mueller · 2024-08-08T14:59:44Z

@brisvag you mean the title of the PR?

brisvag · 2024-08-08T15:27:43Z

I mean the description of the pr, which still refers to fixed_width and some other changes that are not in here anymore.

jo-mueller · 2024-08-09T10:50:44Z

@brisvag should be correct now - I updated the gif and all the descriptions :)

brisvag · 2024-08-09T12:23:12Z

Idea (but also ok to leave for followup): update the example export_figure.py to include enabling the scale bar and setting a fixed length, since this is probably the typical application!

Co-Authored-By: Lorenzo Gaifas <brisvag@gmail.com>

jo-mueller · 2024-08-09T15:38:53Z

@brisvag

update the example export_figure.py to include enabling the scale bar and setting a fixed length

Just added it. But for future reference, this may look not so nice in the high-res screenshot due to #6758.

brisvag · 2024-08-09T15:55:51Z

Good to flag it, thanks! I think @melonora's on it already :)

examples/export_figure.py

psobolewskiPhD

Late to the party, but love it.
length makes sense to me as the name.

Because it came to my mind playing with this, for a followup PR: it should be possible to disable the text, so show just the scale bar -- then in the legend you say all the scale bars are 50 um or whatever and don't deal with text clipping the scale bar (make the font large and zoom out a bit).

I'm not sure adding this to the export_figure example makes much sense for this, because it uses reset_view, so you can't get a zoomed view. This is more for a figure with two different zoom levels? Like an inset for example.
What we need to do--in a followup PR--is update the scale_bar example to include all the cool new shit you can do. It's been on my todo #7007 👀

Co-Authored-By: Lorenzo Gaifas <brisvag@gmail.com>

jo-mueller · 2024-08-12T06:13:19Z

@psobolewskiPhD I have added it here because I think there's no harm in showing some functionality in multiple different places of the docs. But I also agree, a more comprehensive example on how to use the scale bar would be a great addon to the docs. The export_figure function for instance I didn't know but it's very helpful for me :)

Czaki · 2024-08-12T11:35:51Z

Hm. Maybe it will be a nice improvement to change the length parameter from float to pint.Qunatity). To use one parameter to define the length of scale bar?

brisvag · 2024-08-12T13:43:32Z

I think it could go either way... but maybe two parameters of simple types is more ergonomic to work with, since most people are not familiar with pint?

jo-mueller added 5 commits August 7, 2024 16:52

do rounding only when fixed_width is unset

d3a1312

code style

bf95e70

force fixed_width to int

b404824

added comment for fixed_width usage

5750279

github-actions bot added the tests Something related to our tests label Aug 7, 2024

brisvag added the feature New feature or request label Aug 8, 2024

brisvag reviewed Aug 8, 2024

View reviewed changes

examples/scale_bar.py Outdated Show resolved Hide resolved

napari/_vispy/_tests/test_vispy_scale_bar_visual.py Outdated Show resolved Hide resolved

napari/_vispy/overlays/scale_bar.py Outdated Show resolved Hide resolved

jo-mueller added 3 commits August 8, 2024 10:33

removed setting fixed size in example

048e9dd

merged testing functionality for fixed width into existing test

0520ee6

refactored target_canvas_pixels_rounded variable

602616c

jo-mueller requested review from melonora and a team as code owners August 8, 2024 08:38

[pre-commit.ci] auto fixes from pre-commit.com hooks

bbb8893

for more information, see https://pre-commit.ci

brisvag reviewed Aug 8, 2024

View reviewed changes

napari/_vispy/_tests/test_vispy_scale_bar_visual.py Outdated Show resolved Hide resolved

jo-mueller and others added 3 commits August 8, 2024 10:45

Update napari/_vispy/_tests/test_vispy_scale_bar_visual.py

92f7aca

Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>

change property name from fixed_width to length

aa79f2d

[pre-commit.ci] auto fixes from pre-commit.com hooks

27dc76b

for more information, see https://pre-commit.ci

brisvag approved these changes Aug 8, 2024

View reviewed changes

napari/components/overlays/scale_bar.py Outdated Show resolved Hide resolved

jo-mueller mentioned this pull request Aug 8, 2024

Expose scale bar parameters in GUI #7168

Open

Update napari/components/overlays/scale_bar.py

064a049

Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>

brisvag changed the title ~~Add fixed_width parameter to ScaleBarOverlay and `VispyScaleBarOv…~~ Add optional fixe length parameter to scale bar Aug 8, 2024

brisvag reviewed Aug 8, 2024

View reviewed changes

napari/components/overlays/scale_bar.py Outdated Show resolved Hide resolved

brisvag reviewed Aug 8, 2024

View reviewed changes

napari/components/_tests/test_scale_bar.py Outdated Show resolved Hide resolved

melonora approved these changes Aug 8, 2024

View reviewed changes

jo-mueller and others added 2 commits August 8, 2024 11:37

Update napari/components/_tests/test_scale_bar.py

2a17fa0

Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>

set length type to float

2677f4d

brisvag added the topic:vispy Vispy integration label Aug 8, 2024

brisvag added this to the 0.5.2 milestone Aug 8, 2024

set default target length to float

34ced1c

Co-Authored-By: Lorenzo Gaifas <brisvag@gmail.com>

set _target_length to float

497122b

Co-Authored-By: Lorenzo Gaifas <brisvag@gmail.com>

Merge branch 'main' into add-fixed-width

e1222d3

add scale bar to example script

468b509

Co-Authored-By: Lorenzo Gaifas <brisvag@gmail.com>

jo-mueller requested review from melissawm, psobolewskiPhD and lucyleeow as code owners August 9, 2024 15:38

brisvag reviewed Aug 9, 2024

View reviewed changes

examples/export_figure.py Outdated Show resolved Hide resolved

psobolewskiPhD approved these changes Aug 9, 2024

View reviewed changes

moved scale bar creation up in example

0309623

Co-Authored-By: Lorenzo Gaifas <brisvag@gmail.com>

brisvag added the ready to merge Last chance for comments! Will be merged in ~24h label Aug 12, 2024

jni merged commit fb98ee0 into napari:main Aug 12, 2024
39 of 40 checks passed

github-actions bot removed the ready to merge Last chance for comments! Will be merged in ~24h label Aug 12, 2024

jni added the highlight PR that should be mentioned in next release notes label Aug 12, 2024

jo-mueller deleted the add-fixed-width branch August 13, 2024 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add optional fixed `length` parameter to scale bar #7167

Add optional fixed `length` parameter to scale bar #7167

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add optional fixed length parameter to scale bar #7167

Add optional fixed length parameter to scale bar #7167

Uh oh!

Conversation

Uh oh!

Description

Changes

Checklist

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add optional fixed `length` parameter to scale bar #7167

Add optional fixed `length` parameter to scale bar #7167