Skip to content

Conversation

@jdtournier
Copy link
Member

Addresses issue raised on the forum, caused by presence of non-finite values within data voxels included in the analysis mask. Was previously relying on outlier rejection to deal with this, but this can't work since Inf*0 = NaN...

@jdtournier jdtournier added the bug label Mar 9, 2021
@jdtournier jdtournier added this to the 3.0.3 hotfix milestone Mar 9, 2021
@jdtournier jdtournier requested a review from a team March 9, 2021 11:29
@jdtournier jdtournier self-assigned this Mar 9, 2021
@Lestropie
Copy link
Member

So to be precise, if any individual ODF image contains a non-finite value in a voxel within the user-specified mask, the magnitude of that particular ODF in that particular voxel will instead be interpreted as 0. While this will allow execution, it's maybe not the ideal solution. Perhaps preferable would be for:

  • The initial mask in the first iteration to be the intersection between the user-specified mask and the set of voxels for which all ODFs are finite;
  • A warning to be issued to the user if the mask they provided contains any non-finite values in any ODF.

This could be pursued on dev independently of a quick fix to master?

@Lestropie Lestropie modified the milestones: 3.0.4 hotfix, 3.0.3 hotfix Jun 25, 2021
@thijsdhollander
Copy link
Contributor

Hi @jdtournier, it might be useful as part of this to update the reference with our recent abstract ( https://www.researchgate.net/publication/350248989_Multi-tissue_log-domain_intensity_and_inhomogeneity_normalisation_for_quantitative_apparent_fibre_density ), so users have up to date access to the description and performance evaluation of the current algorithm. This wouldn't affect behaviour of course, so is a safe change. Thanks, Thijs.

@jdtournier
Copy link
Member Author

Sure, will do.

@jdtournier jdtournier force-pushed the mtnormlise_handle_nonfinite_values_in_mask branch from e3c4281 to 54b0bd6 Compare July 8, 2021 14:52
@jdtournier
Copy link
Member Author

jdtournier commented Jul 8, 2021

... if any individual ODF image contains a non-finite value in a voxel within the user-specified mask, the magnitude of that particular ODF in that particular voxel will instead be interpreted as 0

OK, I had a good look into addressing that comment, and I'm pretty sure 1fabe21 does it. Basically, non-finite values will still be loaded as-is (not set to zero), but will inherently be sent to the bottom (for -Inf & NaN) or bottom (for Inf) of the range, and be excluded by the outlier rejection (have its weight set to zero). The code then has to handle the outliers properly, since multiplying by zero is not sufficient to get rid of non-finite values - the changes explicitly sets these values to zero rather than multiplying by zero, which then ensures the balance factors are computed correctly (this is what was previously failing).

Finally, I've added a warning if any non-finite values are detected within the mask. I've also verified that the command produces identical results to what it did before. This remains the case even if a non-finite value is included in the mask, but was previously excluded as an outlier (so the final mask is the same as before anyway). If I introduce a NaN in a voxel that would otherwise have been included in the final mask, the command runs as expected, with changes commensurate with the omission of that voxel (changes of the order of around 1e-8 or so in that case).

So I think this one is good to go...

@jdtournier jdtournier enabled auto-merge July 14, 2021 08:34
Copy link
Member

@Lestropie Lestropie left a comment

Choose a reason for hiding this comment

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

Given the way in which it is now used, i.e. not directly multiplicative, "weights" isn't really a great variable name; it's a mask that imposes the outlier rejection & non-finite value masking on top of the pre-defined user mask. It's fine, but would be worth revising if the command is modified further in the future, e.g. using a two-pass approach to pre-exclude voxels with non-finite values.

@jdtournier jdtournier merged commit ca8c735 into master Jul 15, 2021
@jdtournier jdtournier deleted the mtnormlise_handle_nonfinite_values_in_mask branch July 15, 2021 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants