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

Set thickness threshold based on input range #332

Merged

Conversation

gselzer
Copy link
Contributor

@gselzer gselzer commented Jun 14, 2023

Ran into this issue working on a new napari-imagej use case

ThicknessWrapper ensures that the input image is binary, but it makes the assumption on a threshold which may not make sense based on the input image's unique values.

This PR adjusts the threshold when running the plugin, such that the threshold is halfway between the two unique values.

@alessandrofelder
Copy link
Member

Just to make sure I've understood this correctly @gselzer: another way of describing your proposed change is that this means the BoneJ thickness wrapper will work correctly on a binary image with something unusual like 42 as foreground and 11 as background, which it currently doesn't?

@gselzer
Copy link
Contributor Author

gselzer commented Jun 26, 2023

Yes, that is the intent. I was testing locally, under the hood, the data elements were 8-bit ints of values 0 and 1, but the math should make it work with two arbitrary values.

Previously, the threshold was 128 always, now it should be the mean of the two, rounding up (such that if the values are 0 and 255 as assumed before, the threshold is still 128)

@alessandrofelder alessandrofelder self-requested a review June 26, 2023 16:29
Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

Cool! Thanks @gselzer

@mdoube
Copy link
Member

mdoube commented Jul 6, 2023

I really don't like this exposed at the BoneJ user level, although it's correct at the lower level (Bob Dougherty's implementation handles arbitrary, user-set thresholds): the reason is that it is much cleaner to communicate with users that 0 is background and 255 is foreground, across the board, and if some plugins work with some other pair of values it breaks the expectation when other plugins don't work with that pair of values, or else you have to set this same arbitrary threshold across all the plugins that are going to work on the same image.

@kephale
Copy link

kephale commented Jul 6, 2023

@mdoube understood. The underlying problem is certainly that IJ1 uses the 0/255 convention to implement a binary.

Just to be clear about the impact of this choice:

It seems like you are favoring IJ1 usage and that makes sense because, presumably, it is the larger part of BoneJ's user base at the moment. One thing that is important to point out: BoneJ's LocalThickness currently does work with values other than 0/255 and it gives the "wrong" answer in some of those cases.

All of that said, someone who is desperate to not convert to 0/255 can certainly follow your suggestion of using Bob Dougherty's implementation directly (https://github.com/fiji/LocalThickness/blob/master/src/main/java/sc/fiji/localThickness/LocalThicknessWrapper.java).

@ctrueden
Copy link
Member

ctrueden commented Jul 6, 2023

@mdoube Is it the intent that these routines only work with 0/255 images? If so, would it make sense to add consistency checks to fail fast when the routine is fed a non-0/255 binary image, and inform the user? That would be in line with your "communicate with users that 0 is background and 255 is foreground" preference, no?

If this is indeed the intent, to prevent these routines from working with non-0/255 images, e.g. ImgLib2 BitType images, then I fear we will not be able to use such routines in the PyImageJ use case, since we really need support for BitType. In that case, a (longer term) solution might be to fork the BoneJ2 ops into the ImageJ Ops2 library currently under development, with small adjustments to make it behave better with ImgLib2 data structures.

@mdoube
Copy link
Member

mdoube commented Jul 7, 2023

BoneJ's LocalThickness currently does work with values other than 0/255 and it gives the "wrong" answer in some of those cases.

Thanks for pointing this out @kephale. I have been bashing about with Local_Thickness in the last couple of days and will take a look at this. I see that the BoneJ2 check on ImagePlus input data only looks for two distinct values, rather than enforcing that the two values are 0/255, which is general but does not enforce the assumption. This is a bug that I will fix.

Hi @ctrueden

Is it the intent that these routines only work with 0/255 images?

Yes, that's the assumption.

would it make sense to add consistency checks to fail fast when the routine is fed a non-0/255 binary image, and inform the user

For Legacy plugins I implemented an isBinary() method which makes sure that all the pixels are either 0 or 255, and is typically called early on in the plugin's run() method. It seems to be pretty fast; at least it is not obtrusively slow.

Richard and I implemented something like this for the BoneJ2 Modern plugins but it turns out to be impossible to make it always fail fast: there is (or was at the time) no way of assuming that all the pixels belong exclusively to either 0 or 255 (or indeed any arbitrary pair of values) without checking them all. This means in practice that all the pixels have to be checked for a truly binary image, while a non-binary image may fail faster because a third value usually turns up early in the checking. For large (several GB) images this can add several seconds of waiting around time (with the Modern cursor.next() style notably slower than the ImageJ1 style). So I see the point of a dedicated BitType binary image type.

we really need support for BitType

Could you provide conversion routines to handle the ImageJ1 standard assumption (0/255, e.g. the output of Image > Adjust > Threshold), and also all the exotic possibilities suggested above? What I was alluding to in my response to @gselzer is that BoneJ has started from the assumption that users have images resulting from binary segmentation ImageJ1-style, which is 8-bit 0/255, and that the same binary image is fed to all the plugins (a user might want to calculate all of BV/TV, Tb.Sp, Conn.D and Anisotropy on the same image, one after the other, in a script). Handling other things requires a wholesale rewrite of all the plugins: they often have low level pixel value checking for background/foreground that assumes 0/255.

fork the BoneJ2 ops into the ImageJ Ops2 library

Sure, that would be great and was the intended direction of travel as we figured out which BoneJ Ops had stability, performance and general interest that would justify the larger audience. Just bear in mind that quite a bit of the useful parts of BoneJ are still Legacy ImageJ1 style (like Thickness), and that the Ops style can be quite a bit slower than the Legacy style. Case in point: I tried very hard to make Modern Connectivity as fast as the Legacy version, but even after squeezing it hard and multithreading etc. it is still 4× slower on my machine.

umzc_378p_Apteryx_haastii_head.zip

//warmup
for (i = 0; i < 5; i++){
	run("Connectivity");
	run("Connectivity (Modern)", "inputimage=net.imagej.ImgPlus@585b5dbd");
}

//timed runs
for (i = 0; i < 5; i++){
	start = getTime();
	run("Connectivity");
	end = getTime();
	duration = end - start;
	print("Legacy connectivity finished in "+ duration +" ms");
}


for (i = 0; i < 5; i++){
	start = getTime();
	run("Connectivity (Modern)");
	end = getTime();
	duration = end - start;
	print("Modern connectivity finished in "+ duration +" ms");
}

Legacy connectivity finished in 486 ms
Legacy connectivity finished in 487 ms
Legacy connectivity finished in 480 ms
Legacy connectivity finished in 521 ms
Legacy connectivity finished in 472 ms
Modern connectivity finished in 2065 ms
Modern connectivity finished in 1872 ms
Modern connectivity finished in 2010 ms
Modern connectivity finished in 2040 ms
Modern connectivity finished in 2049 ms

mdoube pushed a commit that referenced this pull request Jul 13, 2023
* Set threshold based on input maximum

* Take minimum into account for threshold
mdoube pushed a commit that referenced this pull request Apr 5, 2024
* Set threshold based on input maximum

* Take minimum into account for threshold
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.

5 participants