Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Figure_shape in imagewidget and related histogram widget bug fix #555

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

Merged
merged 4 commits into from
Jul 18, 2024

Conversation

apasarkar
Copy link
Collaborator

@apasarkar apasarkar commented Jul 15, 2024

closes #553, closes #554

…pe logic in imagewidget to deal with multiple shape values in constructor
@apasarkar apasarkar marked this pull request as draft July 15, 2024 18:23
fastplotlib/widgets/histogram_lut.py Show resolved Hide resolved
fastplotlib/widgets/histogram_lut.py Show resolved Hide resolved
fastplotlib/widgets/image.py Outdated Show resolved Hide resolved
@kushalkolar
Copy link
Member

Thanks! Is the histogram calc the only thing that was causing issues on your end?

@apasarkar
Copy link
Collaborator Author

apasarkar commented Jul 16, 2024

@kushalkolar I think one thing to note is that the histogram computation right now is nice if the user's data is loaded into RAM. If otoh the user is using ImageWidge to visualize on-the-fly motion correction, things get trickier because the image array object is doing the extra work of loading from disk. In this case, the histogram computations will involve a lot of extra work (loading a large chunk of data into memory just to estimate a histogram of the data). Maybe in that case the user should be able to just specify "absolute" vmin and vmax values without a histogram. I find that knowing the range of the data (via some scale bars) is very useful, even if there is no histogram.

@kushalkolar
Copy link
Member

kushalkolar commented Jul 17, 2024

@kushalkolar I think one thing to note is that the histogram computation right now is nice if the user's data is loaded into RAM. If otoh the user is using ImageWidge to visualize on-the-fly motion correction, things get trickier because the image array object is doing the extra work of loading from disk. In this case, the histogram computations will involve a lot of extra work (loading a large chunk of data into memory just to estimate a histogram of the data). Maybe in that case the user should be able to just specify "absolute" vmin and vmax values without a histogram. I find that knowing the range of the data (via some scale bars) is very useful, even if there is no histogram.

Ah so you mean like the lazy arrays you have? What about allowing HistogramLUTWidget to take a pre-computed histogram?

What about data that can be lazy loaded, such as memmaps? We only use a max of 500 x 100 x 100 pixels for computing the histogram, data is subsampled to meet that max number of pixels:

https://github.com/apasarkar/fastplotlib/blob/90af0d4a136d918f99551b3fead28773693e40cf/fastplotlib/widgets/histogram_lut.py#L140-L152

We could benchmark this.

If we add a kwarg to pass in a pre-computed histogram, we can just pass an "empty" histogram with all zero bins, but have a vmin and vmax so that it can be interacted with. Or a histogram=False kwarg that will not compute the histogram and just make a "vmin-vmax" widget, i.e. exactly same thing but it just makes all bins zero without you having to pass in a precomputed one.

@kushalkolar
Copy link
Member

is this good to go? LGTM. The other points, such as just operating as a vmin-vmax widget can be done in a later PR

@kushalkolar kushalkolar marked this pull request as ready for review July 18, 2024 02:43
@apasarkar
Copy link
Collaborator Author

@kushalkolar Missed your comment about the vmin/vmax. Strongly agree we need something like that -- I will submit a pull for it.

@apasarkar apasarkar merged commit 5458316 into fastplotlib:main Jul 18, 2024
15 checks passed
@apasarkar apasarkar deleted the iw_hist_and_kwargs branch July 18, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure_shape inconsistency in ImageWidget Histogram zero values
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.