Make point size isotropic #5582

brisvag · 2023-02-22T15:13:50Z

Description

With vispy 0.12 (#5312) we introduced some changes to how point sizes are computed. Specifically, the scale is no longer used to determine the size.

The initial reason was #2213. For a long explanation (with all the caveats and complications), see vispy/vispy#2453 (specifically, this comment has a summary of the behaviour).

While the current state not ideal (and we're trying to figure out over at vispy if we can solve it), this at least solves nasty problems like #2213. However, it means that a size of 10 is still 10 even if the scale of the layer is 5 (and not 50).

This may "break" some code that relies on this transformation; in napari, one of the consequences is that the new points layer button on the viewer gui will now create tiny (or massive) points if the scale is different.

To test, try this:

import napari
import numpy as np

v = napari.Viewer()
v.add_image(np.random.rand(100, 100), scale=[10, 10])

then click the "new points" button and try to annotate some points manually. On main, they'll be so small that they're invisible.

This also exposed another issue (which became the main point of this PR): while point sizes are currently anisotropic, this information is not properly used in many places (visualisation being the primary); not only it's mostly unused, but it's handwaved in a few places (we arbitrarily take the average size over each dimension to determine the visualised size, we only allow setting isotropic sizes from gui and the current_size, and so on).

Ultimately, we decided that we can't reasonably support anisotropic sizes, so we should do away with them. Most changes in this PR have to do with that.

Type of change

Bug-fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

codecov · 2023-02-22T15:33:34Z

Codecov Report

Merging #5582 (c154f0c) into main (37c7ec2) will decrease coverage by 0.02%.
The diff coverage is 95.55%.

@@            Coverage Diff             @@
##             main    #5582      +/-   ##
==========================================
- Coverage   91.47%   91.45%   -0.02%     
==========================================
  Files         571      571              
  Lines       49771    49710      -61     
==========================================
- Hits        45527    45463      -64     
- Misses       4244     4247       +3

Impacted Files	Coverage Δ
napari/_vispy/layers/points.py	`94.01% <90.00%> (+0.26%)`	⬆️
napari/layers/points/points.py	`97.71% <90.00%> (-0.39%)`	⬇️
.../_qt/layer_controls/_tests/test_qt_points_layer.py	`100.00% <100.00%> (ø)`
napari/layers/points/_slice.py	`100.00% <100.00%> (ø)`
napari/layers/points/_tests/test_points.py	`100.00% <100.00%> (ø)`
napari/utils/_tests/test_misc.py	`100.00% <100.00%> (ø)`

psobolewskiPhD · 2023-02-24T21:30:03Z

Maybe I'm missing something or perhaps it's a macOS thing (or arm64 thing), but the example from OP look fine to me? I mean the points are small, but on the same order as the pixels? If you set a face color to not white or black it's good?

I can make them bigger by increasing the size...

And with this PR it seems no different?

brisvag · 2023-03-01T09:36:12Z

Maybe I shoudl have given a more extreme example. If you use a scale of (1000, 1000), they will be hardly visible (to the point I thought there was a visualisation bug until I realized the issue). Most importantly, the behaviour is different from pre-vispy012, where regardless of scale you'd always get relatively big points when you clicked that button.

psobolewskiPhD · 2023-03-01T09:48:36Z

With larger image, indeed on main the points are invisible. But I still see no difference in behavior when I switch to this PR branch 😢

…-points

brisvag · 2023-03-07T09:06:55Z

It worked when setting things manually, but not once I put it in the layer initialization. Turns out, the way we initialize size in Points simply ignores the given size if it's not a scalar...

EDIT: it's worse; while size is an (n, D) array to allow anisotropicity, _current_size only stores a scalar :/

brisvag · 2023-03-07T10:43:22Z

I'm surprised I never encountered issue here... But I'm not sure how to solve. We also expose size in the gui as a single value. To be honest, I'm not sure why we have anisotropic sizes in the first place, since they cause several issues and we're not even able to display them. AFAIK out_of_slice_display is the only place where anisotropic sizes are relevant... which is a bit weird in the first place :/

Czaki · 2023-03-10T14:04:45Z

Should a similar change be also introduced in the context of add points in the constructor?

I think also about the scenario of saving and load points via csv.

brisvag · 2023-03-10T14:46:43Z

Should a similar change be also introduced in the context of add points in the constructor?

This is tricky, and comes with all the issues that we had when discussing #4705.

Czaki · 2023-03-10T16:20:34Z

Why cannot calculate the size of the layer property and calculate it based on layer extent.steep?

brisvag · 2023-03-10T17:02:53Z

Why cannot calculate the size of the layer property and calculate it based on layer extent.steep?

Not sure I follow... the main issue is that we're missing a MISSING value that we can use to determine what to do when the layer gets added to the viewer.

Czaki · 2023-03-10T17:06:37Z

As I understand, the problem is that Vispy ignores the scale parameter of point.
So why do not apply scale before passing points to vispy?

brisvag · 2023-03-10T22:34:16Z

I think we're losing track of the issue here :P

The problem was initially that anisotropic scales were causing issues because they make no sense for non-deformable objects like markers. This was apparent in your issue in #2213. So I disabled that in vispy by having the scale no longer affect the size of the point.

Now, this revealed an extra problem, which is that not just scale, but also size can be anisotropic, which has exactly the same problem.

I'm wondering if it makes sense to keep that as it it, since we can't even render anisotropic markers, so the only effect that anisotropic size has is to affect out of slice display.

Czaki · 2023-03-10T22:50:23Z

For me using min(scale[-2:]) could be a good heuristic to calculate the scale for the size of points.

brisvag · 2023-03-13T10:36:53Z

For me using min(scale[-2:]) could be a good heuristic to calculate the scale for the size of points.

Why not max or mean though? Any of these is pretty arbitrary, and might be surprising to the user.

brisvag · 2023-03-14T14:50:59Z

Notes from chatting with @Czaki:

even having anisotropic size does not solve it cause to pass to vispy we need to multiply by scale

Maybe a good heuristic is to just use the last dimension. It solves the problem now, and anisotropic scales/sizes are the only ones affected (which anyways don't work on main).

We're still left with a better situation compared to #2213 because at least points don't zoom in/out when rotating.

…-points

brisvag · 2023-04-12T14:09:55Z

@andy-sweet and @mstabrin: git says you both touched this code quite a bit, so you might be interested in checking this!

I'm a bit confused about the failing tests...

napari/_qt/widgets/qt_viewer_buttons.py

napari/layers/points/_tests/test_points.py

napari/layers/points/points.py

…-points

brisvag · 2023-06-02T09:51:32Z

This is ready to review and merge now :)

andy-sweet

Pretty much looks good and seemed to work as advertised for me. Only request is to fix/clarify the extent augmented issue.

andy-sweet · 2023-06-05T16:27:40Z

napari/_vispy/layers/points.py

@@ -60,20 +57,23 @@ def _on_data_change(self):

        set_data = self.node._subvisuals[0].set_data

+        # use only last dimension to scale point sizes, see #5582
+        scale = self.layer.scale[-1]


Optional: my intuitive pick for this would be the root of the volume scaling factor of the transform associated with the node. But this is better than nothing, should work for common cases, and I don't feel strongly here.

root of the volume scaling factor of the transform associated with the node

You mean for all dimensions? but yeah we discussed around this before, every option has some downside, so we settled on this for now and we'll see if people complain :P

andy-sweet · 2023-06-05T16:34:44Z

napari/layers/points/points.py

-                self.size[i, :] = (self.size[i, :] > 0) * size
-            self._clear_extent_augmented()
+            idx = np.fromiter(self.selected_data, dtype=int)
+            # TODO: explain why this check; only if size > 0?


Is this a TODO for you or for someone in the future? I can't explain the need for the check either, so I'd also be down to simplify it to self.size[idx] *= size and remove the TODO.

I added it because I didn't understand this check but it was already there, so I left it for the future. However, I now played around a bit, and I'm pretty sure it's wrong (we definitely want to allow points to have size 0, and nothing seems to break on the vispy side when doing so... I'm running tests now).

napari/layers/points/points.py

brisvag · 2023-06-06T08:28:41Z

While remove the "todo" above, I also simplified the code in two related places as I was trying to understand how it worked. They are just refactors, so nothing should change.

andy-sweet · 2023-06-06T14:17:34Z

While remove the "todo" above, I also simplified the code in two related places as I was trying to understand how it worked. They are just refactors, so nothing should change.

I think you forgot to push?

…-points

andy-sweet

Looks good to me now. I haven't had a chance to retest this manually since the last commit, but I assume all is good.

brisvag · 2023-06-07T16:24:00Z

@Czaki: this is a pretty big backward-incompatible change; however, there is an argument to be made for including this if #5312 is included in v0.4.18, because the points size behaviour for anyone who uses isotropic sizes (=most users) will then be more consisent between 0.4.17 and 0.4.18.

With vispy 0.12 (#5312) we introduced some changes to how point sizes are computed. Specifically, the `scale` is no longer used to determine the size. The initial reason was #2213. For a long explanation (with all the caveats and complications), see vispy/vispy#2453 (specifically, [this comment has a summary of the behaviour](vispy/vispy#2453 (comment))). While the current state not ideal (and we're trying to figure out over at vispy if we can solve it), this at least solves nasty problems like scale of the layer is `5` (and not `50`). This may "break" some code that relies on this transformation; in napari, one of the consequences is that the `new points layer` button on the viewer gui will now create tiny (or massive) points if the scale is different. To test, try this: ```py import napari import numpy as np v = napari.Viewer() v.add_image(np.random.rand(100, 100), scale=[10, 10]) ``` then click the "new points" button and try to annotate some points manually. On `main`, they'll be so small that they're invisible. --- This also exposed another issue (which became the main point of this PR): while point sizes are currently anisotropic, this information is not properly used in many places (visualisation being the primary); not only it's mostly unused, but it's handwaved in a few places (we arbitrarily take the average size over each dimension to determine the visualised size, we only allow setting isotropic sizes from gui and the `current_size`, and so on). Ultimately, we decided that we can't reasonably support anisotropic sizes, so we should do away with them. Most changes in this PR have to do with that.  - [x] Bug-fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update --------- Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com> Co-authored-by: Wouter-Michiel Vierdag <w-mv@hotmail.com> Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

# Description With vispy 0.12 (napari#5312) we introduced some changes to how point sizes are computed. Specifically, the `scale` is no longer used to determine the size. The initial reason was napari#2213. For a long explanation (with all the caveats and complications), see vispy/vispy#2453 (specifically, [this comment has a summary of the behaviour](vispy/vispy#2453 (comment))). While the current state not ideal (and we're trying to figure out over at vispy if we can solve it), this at least solves nasty problems like napari#2213. However, it means that a size of `10` is still `10` even if the scale of the layer is `5` (and not `50`). This may "break" some code that relies on this transformation; in napari, one of the consequences is that the `new points layer` button on the viewer gui will now create tiny (or massive) points if the scale is different. To test, try this: ```py import napari import numpy as np v = napari.Viewer() v.add_image(np.random.rand(100, 100), scale=[10, 10]) ``` then click the "new points" button and try to annotate some points manually. On `main`, they'll be so small that they're invisible. --- This also exposed another issue (which became the main point of this PR): while point sizes are currently anisotropic, this information is not properly used in many places (visualisation being the primary); not only it's mostly unused, but it's handwaved in a few places (we arbitrarily take the average size over each dimension to determine the visualised size, we only allow setting isotropic sizes from gui and the `current_size`, and so on). Ultimately, we decided that we can't reasonably support anisotropic sizes, so we should do away with them. Most changes in this PR have to do with that. ## Type of change  - [x] Bug-fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update --------- Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com> Co-authored-by: Wouter-Michiel Vierdag <w-mv@hotmail.com> Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

imagesc-bot · 2023-07-08T08:17:46Z

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/anisotropic-point-sizes/83388/2

…5802) # Description Scale layer interactivity and visualisation are currently not accounting for the layer scale. Try the following: ```py import numpy as np import napari v = napari.Viewer() il = v.add_image(np.random.rand(100, 100)) sl = v.add_shapes([[0, 0], [1, 1]], shape_type='path', scale=[100, 100], edge_width=0.1) ``` You'll see a few issues: - highlight line is not in screen space as advertised. (This is actually a problem with points as well) - the `rotate` handle is way too far because its position is also calculated not accounting for scale - interacting with the shape in any mode is basically impossible, cause coordinates do not account for the scaling of the layer. This PR basically adds a `/ layer.scale[-1]` in several places to get back into correct "sceen space". Note that I used the last dimension as we cannot do anisotropic sizes, just like we recently chose to do with points in #5582. This PR also fixes #4538 by changing the logic of how the "minimum drag" is calculated. PS: ideally we want to transition to using the `SelectionOverlay` in the future, but this is a much bigger effort. Also fixes #5752. ## Type of change  - [x] Bug-fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update

…5802) Scale layer interactivity and visualisation are currently not accounting for the layer scale. Try the following: ```py import numpy as np import napari v = napari.Viewer() il = v.add_image(np.random.rand(100, 100)) sl = v.add_shapes([[0, 0], [1, 1]], shape_type='path', scale=[100, 100], edge_width=0.1) ``` You'll see a few issues: - highlight line is not in screen space as advertised. (This is actually a problem with points as well) - the `rotate` handle is way too far because its position is also calculated not accounting for scale - interacting with the shape in any mode is basically impossible, cause coordinates do not account for the scaling of the layer. This PR basically adds a `/ layer.scale[-1]` in several places to get back into correct "sceen space". Note that I used the last dimension as we cannot do anisotropic sizes, just like we recently chose to do with points in #5582. This PR also fixes #4538 by changing the logic of how the "minimum drag" is calculated. PS: ideally we want to transition to using the `SelectionOverlay` in the future, but this is a much bigger effort. Also fixes #5752.  - [x] Bug-fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update

use current scales to set point size

c4d0472

github-actions bot added the qt Relates to qt label Feb 22, 2023

github-actions bot assigned brisvag Feb 22, 2023

Merge branch 'main' of https://github.com/napari/napari into fix/tiny…

fb88f5c

…-points

brisvag added 5 commits April 6, 2023 14:43

Merge branch 'main' of https://github.com/napari/napari into fix/tiny…

1444705

…-points

Merge branch 'main' of https://github.com/napari/napari into fix/tiny…

1ede28a

…-points

make point size isotropic

5ff38e3

deprecate anisotropic sizes

b98eb30

fix tests, reinstate anisotropic mask

0c4d487

github-actions bot added the tests Something related to our tests label Apr 12, 2023

brisvag changed the title ~~Use current scale to set new point layer size~~ Make point size isotropic Apr 12, 2023

andy-sweet reviewed Apr 12, 2023

View reviewed changes

napari/_qt/widgets/qt_viewer_buttons.py Outdated Show resolved Hide resolved

andy-sweet reviewed Apr 12, 2023

View reviewed changes

napari/layers/points/_tests/test_points.py Outdated Show resolved Hide resolved

andy-sweet reviewed Apr 12, 2023

View reviewed changes

napari/layers/points/points.py Outdated Show resolved Hide resolved

brisvag added 4 commits May 30, 2023 14:05

Merge branch 'main' into fix/tiny-points

0989ca2

Merge branch 'main' of https://github.com/napari/napari into fix/tiny…

2e57818

…-points

fix broadcasting and wrong get_value3d dimensionality

3d9e126

remove leftovers and fix examples

0c0e962

brisvag requested review from andy-sweet, Czaki and jni June 2, 2023 09:51

andy-sweet requested changes Jun 5, 2023

View reviewed changes

fix issue and simplify some code

05405fb

brisvag mentioned this pull request Jun 6, 2023

Fix point selection highlight #5737

Merged

Merge branch 'main' of https://github.com/napari/napari into fix/tiny…

4ee5973

…-points

andy-sweet approved these changes Jun 6, 2023

View reviewed changes

brisvag added the ready to merge Last chance for comments! Will be merged in ~24h label Jun 7, 2023

Merge branch 'main' into fix/tiny-points

4b10a30

melonora mentioned this pull request Jun 15, 2023

v0.4.18 - release process #5911

Closed

Czaki added this to the 0.4.18 milestone Jun 15, 2023

melonora mentioned this pull request Jun 20, 2023

Use napari Points instead of napari Ellipses when SpatialData Shapes are actually circles scverse/napari-spatialdata#37

Closed

jni added 2 commits June 22, 2023 09:59

Merge branch 'main' into fix/tiny-points

a1a3b58

Merge branch 'main' into fix/tiny-points

c154f0c

jni merged commit b9bd525 into napari:main Jun 22, 2023
33 of 34 checks passed

psobolewskiPhD removed the ready to merge Last chance for comments! Will be merged in ~24h label Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make point size isotropic #5582

Make point size isotropic #5582

brisvag commented Feb 22, 2023 •

edited

codecov bot commented Feb 22, 2023 •

edited

psobolewskiPhD commented Feb 24, 2023

brisvag commented Mar 1, 2023

psobolewskiPhD commented Mar 1, 2023

brisvag commented Mar 7, 2023 •

edited

brisvag commented Mar 7, 2023

Czaki commented Mar 10, 2023

brisvag commented Mar 10, 2023

Czaki commented Mar 10, 2023

brisvag commented Mar 10, 2023

Czaki commented Mar 10, 2023

brisvag commented Mar 10, 2023

Czaki commented Mar 10, 2023

brisvag commented Mar 13, 2023

brisvag commented Mar 14, 2023

brisvag commented Apr 12, 2023

brisvag commented Jun 2, 2023

andy-sweet left a comment

andy-sweet Jun 5, 2023

brisvag Jun 6, 2023

andy-sweet Jun 5, 2023

brisvag Jun 6, 2023

brisvag commented Jun 6, 2023

andy-sweet commented Jun 6, 2023

andy-sweet left a comment

brisvag commented Jun 7, 2023

imagesc-bot commented Jul 8, 2023

Make point size isotropic #5582

Make point size isotropic #5582

Conversation

brisvag commented Feb 22, 2023 • edited

Description

Type of change

codecov bot commented Feb 22, 2023 • edited

Codecov Report

psobolewskiPhD commented Feb 24, 2023

brisvag commented Mar 1, 2023

psobolewskiPhD commented Mar 1, 2023

brisvag commented Mar 7, 2023 • edited

brisvag commented Mar 7, 2023

Czaki commented Mar 10, 2023

brisvag commented Mar 10, 2023

Czaki commented Mar 10, 2023

brisvag commented Mar 10, 2023

Czaki commented Mar 10, 2023

brisvag commented Mar 10, 2023

Czaki commented Mar 10, 2023

brisvag commented Mar 13, 2023

brisvag commented Mar 14, 2023

brisvag commented Apr 12, 2023

brisvag commented Jun 2, 2023

andy-sweet left a comment

Choose a reason for hiding this comment

andy-sweet Jun 5, 2023

Choose a reason for hiding this comment

brisvag Jun 6, 2023

Choose a reason for hiding this comment

andy-sweet Jun 5, 2023

Choose a reason for hiding this comment

brisvag Jun 6, 2023

Choose a reason for hiding this comment

brisvag commented Jun 6, 2023

andy-sweet commented Jun 6, 2023

andy-sweet left a comment

Choose a reason for hiding this comment

brisvag commented Jun 7, 2023

imagesc-bot commented Jul 8, 2023

brisvag commented Feb 22, 2023 •

edited

codecov bot commented Feb 22, 2023 •

edited

brisvag commented Mar 7, 2023 •

edited