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

Fix copy/paste of points #5795

Merged
merged 3 commits into from May 4, 2023
Merged

Conversation

andy-sweet
Copy link
Member

@andy-sweet andy-sweet commented May 1, 2023

Fixes/Closes

Closes #5786

Description

The issue here is actually caused by copying constant color values used for text, which causes issues because a single value has ndim > 0. This PR fixes the issue by specifying the dimensionality of a single color value when copying values.

It might be worth reconsidering the implementation here and/or whether constant encodings are worth handling as a specific optimized case like this. But I think it's worth getting this PR in before a longer consideration.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)

How has this been tested?

  • modified an existing test to reproduce the bug, then fixed it
  • all other tests pass with my change
  • manually tested this by copying and pasting lots of points

@andy-sweet andy-sweet added this to the 0.4.18 milestone May 1, 2023
@github-actions github-actions bot added the tests Something related to our tests label May 1, 2023
@andy-sweet andy-sweet added the bugfix PR with bugfix label May 1, 2023
@brisvag
Copy link
Contributor

brisvag commented May 2, 2023

I'm a bit confused, can you explain the change to test?

@andy-sweet
Copy link
Member Author

I'm a bit confused, can you explain the change to test?

Sorry, the implementation comment in the test is not clear. Does the following help?

    # Use an index of 4 to ensure that constant color values, which are
    # RGBA 4-vectors, are handled correctly when copied, unlike in bug:
    # https://github.com/napari/napari/issues/5786

By trying to copy at an index of 4, we reproduce the bug in #5786, which is caused by the combination of two things.

1. Style encodings share the same code to handle scalar style values (e.g. strings) and non-scalar ones (e.g. RGBA colors).
2. There is some special casing to handle constant encodings, so we avoid storing/manipulating an array of values.

In the long term, I wonder if (2) is really worth it, since that is the major cause here and I think vispy will end up storing an array of RGBA color values anyway.

@brisvag
Copy link
Contributor

brisvag commented May 2, 2023

ok, makes sense; yeah vispy ends up with rgba, so it's definitely fine to not special-case, but no need to do it here :P

@andy-sweet andy-sweet requested a review from brisvag May 2, 2023 20:52
@andy-sweet
Copy link
Member Author

@brisvag: since you've already taken a look and this is small, do you mind reviewing? I updated the explanatory comment in the test as above.

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #5795 (be07737) into main (d91f4ee) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5795      +/-   ##
==========================================
+ Coverage   89.79%   89.87%   +0.08%     
==========================================
  Files         614      614              
  Lines       52196    52196              
==========================================
+ Hits        46870    46913      +43     
+ Misses       5326     5283      -43     
Impacted Files Coverage Δ
napari/layers/utils/text_manager.py 94.69% <ø> (ø)
napari/layers/utils/_tests/test_text_manager.py 100.00% <100.00%> (ø)

... and 13 files with indirect coverage changes

@andy-sweet andy-sweet added the ready to merge Last chance for comments! Will be merged in ~24h label May 3, 2023
@andy-sweet andy-sweet merged commit 9da6d28 into napari:main May 4, 2023
35 checks passed
@andy-sweet andy-sweet deleted the fix-points-color branch May 4, 2023 22:17
@Czaki Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label May 25, 2023
@Czaki Czaki mentioned this pull request Jun 7, 2023
Czaki pushed a commit that referenced this pull request Jun 19, 2023
# Fixes/Closes

Closes #5786

# Description
The issue here is actually caused by copying constant color values used
for text, which causes issues because a single value has `ndim > 0`.
This PR fixes the issue by specifying the dimensionality of a single
color value when copying values.

It might be worth reconsidering the implementation here and/or whether
constant encodings are worth handling as a specific optimized case like
this. But I think it's worth getting this PR in before a longer
consideration.

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# How has this been tested?
- [x] modified an existing test to reproduce the bug, then fixed it
- [x] all other tests pass with my change
- manually tested this by copying and pasting lots of points
Czaki pushed a commit that referenced this pull request Jun 21, 2023
# Fixes/Closes

Closes #5786

# Description
The issue here is actually caused by copying constant color values used
for text, which causes issues because a single value has `ndim > 0`.
This PR fixes the issue by specifying the dimensionality of a single
color value when copying values.

It might be worth reconsidering the implementation here and/or whether
constant encodings are worth handling as a specific optimized case like
this. But I think it's worth getting this PR in before a longer
consideration.

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# How has this been tested?
- [x] modified an existing test to reproduce the bug, then fixed it
- [x] all other tests pass with my change
- manually tested this by copying and pasting lots of points
Czaki pushed a commit that referenced this pull request Jun 21, 2023
# Fixes/Closes

Closes #5786

# Description
The issue here is actually caused by copying constant color values used
for text, which causes issues because a single value has `ndim > 0`.
This PR fixes the issue by specifying the dimensionality of a single
color value when copying values.

It might be worth reconsidering the implementation here and/or whether
constant encodings are worth handling as a specific optimized case like
this. But I think it's worth getting this PR in before a longer
consideration.

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# How has this been tested?
- [x] modified an existing test to reproduce the bug, then fixed it
- [x] all other tests pass with my change
- manually tested this by copying and pasting lots of points
Czaki pushed a commit that referenced this pull request Jun 21, 2023
# Fixes/Closes

Closes #5786

# Description
The issue here is actually caused by copying constant color values used
for text, which causes issues because a single value has `ndim > 0`.
This PR fixes the issue by specifying the dimensionality of a single
color value when copying values.

It might be worth reconsidering the implementation here and/or whether
constant encodings are worth handling as a specific optimized case like
this. But I think it's worth getting this PR in before a longer
consideration.

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# How has this been tested?
- [x] modified an existing test to reproduce the bug, then fixed it
- [x] all other tests pass with my change
- manually tested this by copying and pasting lots of points
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy and paste multiple points doesn't work
3 participants