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 interpolation problem with ints #189

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented Nov 26, 2023

This PR changes interpolate_numbehaviour to fallback to float like a number if the first argument is Interger and the second one is Real.

closes #149
closes #152

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just going to suggest something like this in the napari PR, but you beat me while lunching.
❤️

napari_animation/interpolation/base_interpolation.py Outdated Show resolved Hide resolved
@brisvag brisvag changed the title Fix interpaltion problem with ints Fix interpolation problem with ints Nov 27, 2023
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.

Doiesn't fully solve these issue, right? If you have 2 keyframes and opacity goes from 1 to 2 and they're both integers, you'll get a flip in the middle no?

@@ -1,3 +1,4 @@
*.swp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this end up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I edit these files using vim.
I know that I should have this in global gitingore, but I think we should have default configuration fits for most editors.

I could revert this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me to leave it in, but I think I already have this in my global ignore :P

@Czaki
Copy link
Contributor Author

Czaki commented Nov 27, 2023

Doiesn't fully solve these issue, right? If you have 2 keyframes and opacity goes from 1 to 2 and they're both integers, you'll get a flip in the middle no?

No. This will still return int (so 1 or 2 in this case). There are scenarios when interpolating int should return int (for example for 0, 100, and fraction 0.5 the result should be 50, not 50.0).

@brisvag
Copy link
Contributor

brisvag commented Nov 27, 2023

Yes, but 1 and 2 from a "float" field should return 1.5 at fraction 0.75, not 1. My point being, we should still enforce types in napari.

@Czaki
Copy link
Contributor Author

Czaki commented Nov 27, 2023

Yes, but 1 and 2 from a "float" field should return 1.5 at fraction 0.75, not 1. My point being, we should still enforce types in napari.

I totally agree. This is only a fix to reduce the probability of the problem. As it may be hard to find every place that needs coerce and ensure that any future PR will not introduce new problematic property.

@psobolewskiPhD psobolewskiPhD added the ready-to-merge This PR is approved and can be merged after 24 hour `cooldown` label Nov 28, 2023
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (23c1a3c) 85.91% compared to head (40518ab) 86.06%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   85.91%   86.06%   +0.14%     
==========================================
  Files          26       26              
  Lines        1051     1062      +11     
==========================================
+ Hits          903      914      +11     
  Misses        148      148              

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

@psobolewskiPhD psobolewskiPhD merged commit 95801a5 into napari:main Dec 14, 2023
14 checks passed
@Czaki Czaki deleted the fix_interpolate branch December 14, 2023 19:25
@psobolewskiPhD psobolewskiPhD added bug Something isn't working and removed ready-to-merge This PR is approved and can be merged after 24 hour `cooldown` labels Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fading out image not working Unexpected ValueError in animation when changing gamma parameter.
3 participants