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

mrthreshold behaviour with exotic uses #1481

Open
Lestropie opened this issue Nov 8, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@Lestropie
Copy link
Member

commented Nov 8, 2018

Has some degree of overlap with #205 and #738.

Currently the input to the -percentile option is immediately converted to the equivalent behaviour as if the -top or -bottom option had been used (which select the N top-valued or bottom-valued voxels respectively). However it does this conversion based on the voxel_count() function, which assumes:

  1. There are no non-finite values in the input image.

  2. The -ignorezero option has not also been used.

  3. The -mask option has not also been used (it seems the intention is that it only applies to calculation of the optimal threshold, but it's not inconceivable that a user may try to use this option elsewhere, e.g. calculating a percentile within a provided mask).

There's also the presence of the -toppercent and -bottompercent options in addition to the -percentile option, which are quite clearly intended to behave differently given the code branching: I believe that one is intended to calculate a singular threshold from inside the mask that is then applied to the entire image, whereas the other intends to only highlight voxels within the provided mask. But that's not really reflected in the documentation.

I can also see other areas of code where the behaviour in the presence of non-finite input data may not be as expected.

Given this, the discussion in #205, and the bug report in #738, I think the structure & interface of mrthreshold is in need of close scrutiny, with closer attention paid to the potential confounds listed above. Not going to put the time into fixing bugs before determining whether or not the actual command-line interface is to be changed, since that could plausibly have an influence on the target code structure, as well as whether there's a consensus on the intended behaviour for all possible usages.

@jdtournier

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

Yep, I think a review is in order. This command is ancient and has seen a few changes over the years, so a lot of the structure and interface would be relic-status. Worth a revisit - when we find the time... I guess the best thing to do at this stage is to come up with a replacement for the command-line help page, and go from there...?

@Lestropie

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2018

My concern looking at the code was that it was going to in fact be quite difficult to describe completely accurately exactly what the behaviour of each command-line option was going to be, due to the confounds listed above behaving differently for each command-line option, and therefore describing what we want the behaviour to be might in fact be easier than trying to describe what's there already.


My initial instinct was to stick with a single threshold with the option to invert the output, rather than the "above/below" approach described in #205, with more complex masking (e.g. values within a range) achievable using mrcalc; but not hard against it if it turns out to be the consensus preference.

In that case, the remaining issues are:

  • Redundancy between -percentile and -toppercent / -bottompercent options: The latter two would be removed. Bottom percentile could be achieved with a combination of -percentile and -invert.

  • -bottom option kind of goes against this overarching philosophy; that functionality could be achieved by querying the number of voxels in the image and then using -top combined with -invert; less convenient, but no idea how often it's actually required, and could still be done in a single line of bash.


If the above/below approach is instead used:

  • The code would need to be modified throughout to reflect the fact that there's always the possibility of two thresholds being applied at once:

    • One to select voxels above a value (which could be determined based on optimal threshold / absolute value / percentile / top N);
    • One to select voxels below a value (which could be determined by absolute value / percentile / bottom N).
  • The -invert option would not be necessary, since e.g. -below 50 -above 100 would be equivalent to -above 50 -below 100 -invert. But it also wouldn't be "incorrect" to keep it.

  • -percentile option would disappear in favour of -toppercent / -bottompercent.


Other points to consider (which apply to both possibilities):

  • -nan option could theoretically be removed given one could instead pipe to mrcalc and use the new-ish -replace option; but again it's a trade-off.

  • -ignorezero needs to both be more explicit, and have its behaviour fixed, to indicate that it specifically ignores values of zero when determining the threshold. Whether or not values of zero are then "ignored" during application of the threshold is a different thing, and may indeed warrant its own option (e.g. could be achieved using a subsequent mrcalc / mrmath call).

  • Presence of non-finite values in input needs to be properly handled throughout.

  • Appropriate behaviour in the presence of a 4D input image needs to be determined. Something like the -allvolumes option in mrstats is probably required.

  • -mask option currently applies only to the golden section search for determining an optimal threshold; it does not apply to the calculation of a threshold using any other method, nor does it have an influence during application of said threshold. As previously, if voxels outside the mask should be left out, that can be achieved using e.g. mrcalc, but may not be wholly convenient.

@Lestropie

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

Bumping to prompt discussion since it's received milestone assignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.