8000 Fix features issues with init param and property setter by andy-sweet · Pull Request #5646 · napari/napari · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix features issues with init param and property setter #5646

8000
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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Mar 29, 2023
Merged

Fix features issues with init param and property setter #5646

merged 13 commits into from
Mar 29, 2023

Conversation

andy-sweet
Copy link
Member
@andy-sweet andy-sweet commented Mar 21, 2023 •
edited
Loading

Description

This introduces a feature_defaults init parameter and property setter for the three layer types that support that (points, shapes, and vectors). It also fixes some related issues that occur when adding feature-derived colors when modifying the feature defaults in-place.

Note that the feature_defaults is deliberately a little different to the current_properties setter, as the latter also has the side-effect of setting the feature values of the selected elements to the new current property values. Also, using the feature_defaults setter with keys/columns that are extra/missing compared to features will result in an error.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

References

Closes #5632
Closes #5634

How has this been tested?

  • added new tests to cover bugs and setter
  • all tests pass with my change

@github-actions github-actions bot added the tests Something related to our tests label Mar 21, 2023
@andy-sweet
Copy link
Member Author
andy-sweet commented Mar 21, 2023 •
edited
Loading

Sorry, meant to open this in my fork.

@andy-sweet andy-sweet closed this Mar 21, 2023
@andy-sweet andy-sweet reopened this Mar 21, 2023
@andy-sweet andy-sweet mentioned this pull request Mar 21, 2023
Closed
@andy-sweet andy-sweet closed this Mar 21, 2023
@andy-sweet andy-sweet reopened this Mar 22, 2023
@andy-sweet andy-sweet changed the title Fix features issues with init param and property setter [WIP] Fix features issues with init param and property setter Mar 22, 2023
@andy-sweet andy-sweet mentioned this pull request Mar 22, 2023
Closed
@codecov
Copy link
codecov bot commented Mar 22, 2023 •
edited
Loading

Codecov Report

Merging #5646 (faf9622) into main (b1d7c40) will increase coverage by 0.02%.
The diff coverage is 99.00%.

@@            Coverage Diff             @@
##             main    #5646      +/-   ##
==========================================
+ Coverage   89.81%   89.84%   +0.02%     
==========================================
  Files         611      611              
  Lines       51560    51640      +80     
==========================================
+ Hits        46308    46394      +86     
+ Misses       5252     5246       -6
Impacted Files Coverage Δ
napari/layers/shapes/shapes.py 92.42% <87.50%> (+0.15%) ⬆️
napari/layers/points/_tests/test_points.py 100.00% <100.00%> (ø)
napari/layers/points/points.py 98.84% <100.00%> (+0.01%) ⬆️
napari/layers/shapes/_tests/test_shapes.py 100.00% <100.00%> (ø)
napari/layers/utils/_tests/test_layer_utils.py 100.00% <100.00%> (ø)
napari/layers/utils/layer_utils.py 93.35% <100.00%> (+0.34%) ⬆️
napari/layers/vectors/_tests/test_vectors.py 100.00% <100.00%> (ø)
napari/layers/vectors/vectors.py 97.99% <100.00%> (+0.04%) ⬆️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@andy-sweet andy-sweet requested a review from brisvag March 22, 2023 22:21
@andy-sweet andy-sweet marked this pull request as ready for review March 22, 2023 22:22
@andy-sweet
Copy link
Member Author

@brisvag : added you as a reviewer since you poked the features and found these bugs.

@andy-sweet andy-sweet changed the title [WIP] Fix features issues with init param and property setter Fix features issues with init param and property setter Mar 22, 2023
brisvag
brisvag approved these changes Mar 23, 2023
View reviewed changes
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.

Looks great! I really like the mostly identical implementation across layers... We really need to pick the features work up again ;)

One thing: there's a few codecov warning about the setters not being tested. Maybe it's good to add some test for those as well?

@andy-sweet
Copy link
Member Author
andy-sweet commented Mar 23, 2023 •
edited
Loading

Thanks for the quick review.

One thing: there's a few codecov warning about the setters not being tested. Maybe it's good to add some test for those as well?

Thanks codecov - it makes a fair point.

Can you think of an interesting usage of the features_default setter that's very different to the tests I added?

One thing I might do is to modify those tests. That way they handle setting the default value in the three ways we support (initializer param, in-place modification, property setter) - what do you think? Typically, I don't like testing multiple actions in the same test, but they are at least exercising the same usage/behavior. And I'm wary of adding a very similar (i.e. almost duplicate) test for each of the supported layers.

@andy-sweet
Copy link
Member Author

One thing I might do is to modify those tests. That way they handle setting the default value in the three ways we support (initializer param, in-place modification, property setter) - what do you think?

I did that in faf9622. If you don't like it, I'll revert it and write some other separate tests.

brisvag
brisvag reviewed Mar 24, 2023
View reviewed changes
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.

Yeah that's definitely good enough imo!

@andy-sweet
Copy link
Member Author

I think the CI failure is unrelated and possibly related to #5657 instead. Therefore, this is ready to merge, though I might hold off on merging until that CI action is resolved.

@andy-sweet andy-sweet added bugfix PR with bugfix ready to merge Last chance for comments! Will be merged in ~24h labels Mar 27, 2023
@brisvag
Copy link
Contributor
brisvag commented Mar 29, 2023

I think we can merge, this CI failure is everywhere :)

@brisvag brisvag merged commit 04e0525 into napari:main Mar 29, 2023
@brisvag brisvag removed the ready to merge Last chance for comments! Will be merged in ~24h label Mar 29, 2023
@andy-sweet andy-sweet deleted the feat-feature-defaults-setter branch March 29, 2023 14:42
kcpevey pushed a commit to kcpevey/napari that referenced this pull request Apr 6, 2023
@andy-sweet @kcpevey
Fix features issues with init param and property setter (napari#5646)
b2d70f5 
# Description
This introduces a `feature_defaults` init parameter and property setter
for the three layer types that support that (points, shapes, and
vectors). It also fixes some related issues that occur when adding
feature-derived colors when modifying the feature defaults in-place.

Note that the `feature_defaults` is deliberately a little different to
the `current_properties` setter, as the latter also has the side-effect
of setting the feature values of the selected elements to the new
current property values. Also, using the `feature_defaults` setter with
keys/columns that are extra/missing compared to `features` will result
in an error.

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)
- [x] New featur
8000
e (non-breaking change which adds functionality)

# References
Closes napari#5632
Closes napari#5634

# How has this been tested?
- [x] added new tests to cover bugs and setter
- [x] all tests pass with my change
@Czaki Czaki mentioned this pull request May 1, 2023
Closed
@psobolewskiPhD psobolewskiPhD added this to the 0.4.18 milestone May 1, 2023
@brisvag brisvag mentioned this pull request May 2, 2023
Merged
1 task
@Czaki Czaki mentioned this pull request Jun 7, 2023
Closed
Czaki pushed a commit that referenced this pull request Jun 19, 2023
@andy-sweet @Czaki
Fix features issues with init param and property setter (#5646)
ff229d1 
# Description
This introduces a `feature_defaults` init parameter and property setter
for the three layer types that support that (points, shapes, and
vectors). It also fixes some related issues that occur when adding
feature-derived colors when modifying the feature defaults in-place.

Note that the `feature_defaults` is deliberately a little different to
the `current_properties` setter, as the latter also has the side-effect
of setting the feature values of the selected elements to the new
current property values. Also, using the `feature_defaults` setter with
keys/columns that are extra/missing compared to `features` will result
in an error.

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

# References
Closes #5632
Closes #5634

# How has this been tested?
- [x] added new tests to cover bugs and setter
- [x] all tests pass with my change
Czaki pushed a commit that referenced this pull request Jun 21, 2023
@andy-sweet @Czaki
Fix features issues with init param and property setter (#5646)
888bc0c 
# Description
This introduces a `feature_defaults` init parameter and property setter
for the three layer types that support that (points, shapes, and
vectors). It also fixes some related issues that occur when adding
feature-derived colors when modifying the feature defaults in-place.

Note that the `feature_defaults` is deliberately a little different to
the `current_properties` setter, as the latter also has the side-effect
of setting the feature values of the selected elements to the new
current property values. Also, using the `feature_defaults` setter with
keys/columns that are extra/missing compared to `features` will result
in an error.

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

# References
Closes #5632
Closes #5634

# How has this been tested?
- [x] added new tests to cover bugs and setter
- [x] all tests pass with my change
Czaki pushed a commit that referenced this pull request Jun 21, 2023 
# Description
This introduces a `feature_defaults` init parameter and property setter
for the three layer types that support that (points, shapes, and
vectors). It also fixes some related issues that occur when adding
feature-derived colors when modifying the feature defaults in-place.

Note that the `feature_defaults` is deliberately a little different to
the `current_properties` setter, as the latter also has the side-effect
of setting the feature values of the selected elements to the new
current property values. Also, using the `feature_defaults` setter with
keys/columns that are extra/missing compared to `features` will result
in an error.

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

# References
Closes #5632
Closes #5634

# How has this been tested?
- [x] added new tests to cover bugs and setter
- [x] all tests pass with my change
Czaki pushed a commit that referenced this pull request Jun 21, 2023
@andy-sweet @Czaki
Fix features issues with init param and property setter (#5646)
5ddbdef 
# Description
This introduces a `feature_defaults` init parameter and property setter
for the three layer types that support that (points, shapes, and
vectors). It also fixes some related issues that occur when adding
feature-derived colors when modifying the feature defaults in-place.

Note that the `feature_defaults` is deliberately a little different to
the `current_properties` setter, as the latter also has the side-effect
of setting the feature values of the selected elements to the new
current property values. Also, using the `feature_defaults` setter with
keys/columns that are extra/missing compared to `features` will result
in an error.

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

# References
Closes #5632
Closes #5634

# How has this been tested?
- [x] added new tests to cover bugs and setter
- [x] all tests pass with my change
@andy-sweet andy-sweet mentioned this pull request Jul 11, 2023
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brisvag brisvag brisvag approved these changes

Assignees

@andy-sweet andy-sweet

Labels
56F0 bugfix PR with bugfix tests Something related to our tests
Projects
None yet
Milestone
0.4.18
Development

Successfully merging this pull request may close these issues.

Point color sees changes to feature_defaults "late" Issues with coloring Points by feature with empty layer
3 participants
@andy-sweet @brisvag @psobolewskiPhD
0