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

Rescale image data if outside float32 precision #6537

Merged
merged 37 commits into from
Feb 3, 2024

Conversation

Czaki
Copy link
Collaborator

@Czaki Czaki commented Dec 14, 2023

References and relevant issues

closes #6533

Description

Add check if image data could be visualized using vispy and if not, then rescale data.

@github-actions github-actions bot added the tests Something related to our tests label Dec 14, 2023
@Czaki Czaki changed the title add _coerce_contrast_limits function and use it Rescale image data if outside float32 precision Dec 14, 2023
@Czaki Czaki changed the title Rescale image data if outside float32 precision Rescale image data if outside float32 precision Dec 14, 2023
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dab2a86) 92.29% compared to head (d146dd0) 92.27%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6537      +/-   ##
==========================================
- Coverage   92.29%   92.27%   -0.03%     
==========================================
  Files         604      605       +1     
  Lines       54037    54185     +148     
==========================================
+ Hits        49876    49999     +123     
- Misses       4161     4186      +25     

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

@jnahlers
Copy link
Contributor

Thanks @Czaki, this method looks like it handles the toy example in the 'Steps to Reproduce' of #6533. However, I am still having trouble with my actual images. An example is this image.

@Czaki
Copy link
Collaborator Author

Czaki commented Dec 17, 2023

@jnahlers Your case is different from described in the original issue. In this case, is small contrast limits for big values. But it looks like I could easily fix the code to work with your data.

@jnahlers
Copy link
Contributor

It looks like there is still some kind of overflow happening? Check out this array, specifically around index 50 (i.e. [50,:,:]).

@Czaki
Copy link
Collaborator Author

Czaki commented Dec 17, 2023

Now It should be better. I forgot to cast infinity to float32.

@psobolewskiPhD
Copy link
Member

@Czaki I think that test fail is real related to the uninitialized float64 empty array. Not sure if switching those arrays to zeroes would just mask something?

Czaki and others added 3 commits January 27, 2024 22:32
Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
@Czaki
Copy link
Collaborator Author

Czaki commented Jan 28, 2024

@Czaki I think that test fail is real related to the uninitialized float64 empty array. Not sure if switching those arrays to zeroes would just mask something?

No. The error was the result of mistaking scale and offset. For testability, I move logic to CoercedContrastLimits.

@jnahlers
Copy link
Contributor

@Czaki I am still having trouble with the examples I gave above . Is this the issue of small contrast limits for big values you mentioned above?

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 29, 2024

@jnahlers It looks like I have made a bug when merging main into this PR. I have fixed it now.

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 29, 2024

As far as I could check, the tests are failing now because of NaN. So it looks like using np.empty on float is a bad idea. So it should be either a predefined array or int data.

Random is bad because its slowdown test suite.

cc @psobolewskiPhD

@psobolewskiPhD
Copy link
Member

Even with int could we get something strange, given that fact that it's uninitialized?
Maybe best to use zeros?

Going forward, could we make a set of arrays of different size/shapes using random.random and then reuse them everywhere else?

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 29, 2024

No. Int cannot be something strange.

We could create preset of data using fixtures.

@jnahlers
Copy link
Contributor

@Czaki Ping me if you want to me try the branch again, right now I'm still seeing black on the examples. Thanks for you work on this!

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 30, 2024

@Czaki Ping me if you want to me try the branch again, right now I'm still seeing black on the examples. Thanks for you work on this!

I have removed an important change when removing debug print....

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 30, 2024

This is how it looks on my machine:

napari_scale-2024-01-30_11.59.36.mp4

I do not understand the source of vertical artifacts. But as it looks like thumbnail do not have such artifacts, maybe rescale up could be done smarter.

@Czaki
Copy link
Collaborator Author

Czaki commented Feb 1, 2024

@jnahlers Is it ok now?

@jnahlers
Copy link
Contributor

jnahlers commented Feb 1, 2024

@jnahlers Is it ok now?

Looks good!

@Czaki Czaki added the ready to merge Last chance for comments! Will be merged in ~24h label Feb 1, 2024
@Czaki Czaki merged commit 65e5d38 into napari:main Feb 3, 2024
34 checks passed
@Czaki Czaki deleted the fix_large_images branch February 3, 2024 17:34
@Czaki Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PR with maintance changes, tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canvas goes black for very small or large numbers in image
4 participants