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

Value range determined from whole volume, not first slice #261

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

felixhorger
Copy link

Hi,

this is a useful tool, thanks for developing it!
One minor improvement would be that the value range of grey scale volumes is computed over the whole volume instead of over the first slice.

Cheers,
Felix

@timholy
Copy link
Member

timholy commented Apr 22, 2022

I agree this would improve some things, but I also worry about the case where the first volume might have ~1000 slices:

julia> using ImageView, BenchmarkTools

julia> a = rand(2048, 2048);

julia> @btime ImageView.default_clim($a)
  18.510 ms (2 allocations: 128 bytes)
Observable{CLim{Float64}} with 0 listeners. Value:
CLim{Float64}(3.144755991169035e-7, 0.9999997443110233)

Now multiply that by 1000 and you have 18s. Is it really worth that amount of time? Is there something else we could do instead?

@felixhorger
Copy link
Author

Hi Tim, sorry for taking so long to get back to you!
I wrote a function which is faster than minimum_finite/maximum_finite from Images.jl by a factor of nearly 5.
On my machine the above test then takes only 4ms which would result in four seconds on the huge volume, that's alright I'd say.
Another option would be to recompute the colour range for every slice upon selection, however in my experience that confuses the user (some MRI scanners do that -> flickering when scrolling).
Let me know what you think.
Cheers, Felix

src/ImageView.jl Outdated
elseif minval == maxval
function fast_finite_extrema(a::AbstractArray)
mini = a[1]
maxi = a[1]
Copy link
Member

Choose a reason for hiding this comment

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

What if a[1] isn't finite?

@timholy
Copy link
Member

timholy commented Jan 9, 2023

I think we still might want to limit the number of points considered, e.g., if we've checked more than 10^8 points then I think we'd be safe. But otherwise I like this solution.

@felixhorger
Copy link
Author

Done! :)
What do you think about using threads to speed up the value span determination?

@ViralBShah
Copy link

Is this ok to merge?

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.

3 participants