ENH: Revise the implementation of FuzzyOverlap #2530
Conversation
- [x] Accept calculating overlap within an ``in_mask`` - [x] Drop calculation of the difference image
There was a problem hiding this comment.
Not sure if this is ready for review, but had a look through and saw a few things that look a little redundant. I didn't thoroughly check the math for consistency between the old and new versions. That might be easier if you could make a few comments on your intended differences. (Whenever you're ready for review.)
nipype/algorithms/metrics.py
Outdated
| self._results['jaccard'] = float(np.sum(weights * jaccards)) | ||
| self._results['dice'] = float(np.sum(weights * dices)) | ||
| self._results['class_fji'] = [ | ||
| float(v) for v in jaccards.astype(float).tolist()] |
There was a problem hiding this comment.
jaccards.astype(float).tolist() seems like it should be fine? I think this and the next comprehension are redundant.
There was a problem hiding this comment.
This is me over-worrying about output traits. In the past I've seen that ndarray.astype(float).tolist() returned a list of numpy floats that traits didn't like. I'll check if this is not necessary anymore.
There was a problem hiding this comment.
Ah. If that's the case, then [float(v) for v in jaccards] is another option. There shouldn't be any change to values or iteration by dropping astype(float) or tolist().
nipype/algorithms/metrics.py
Outdated
| if self.inputs.weighting != "none": | ||
| volumes = np.sum((refdata + tstdata) > 0, axis=1).reshape((-1, ncomp)) | ||
| weights = 1.0 / np.array(volumes) |
| numerators = np.atleast_2d( | ||
| np.minimum(refdata, tstdata).reshape((-1, ncomp))) | ||
| denominators = np.atleast_2d( | ||
| np.maximum(refdata, tstdata).reshape((-1, ncomp))) |
There was a problem hiding this comment.
These shapes look guaranteed to be 2D, already.
There was a problem hiding this comment.
If only one item in the input lists are passed, numpy squeezes the redundant dimension. This was necessary for this interface to work.
There was a problem hiding this comment.
Well that's surprising and annoying. Okay.
nipype/algorithms/metrics.py
Outdated
| nb.load(self.inputs.in_ref[0]).header), | ||
| self.inputs.out_file) | ||
| # Fill-in the results object | ||
| self._results['jaccard'] = float(np.sum(weights * jaccards)) |
There was a problem hiding this comment.
I think you can just use the dot product here?
self._results['jaccard'] = weights.dot(jaccards)
|
Do you want to target this for 1.0.3? |
|
Sure, let's do that 👍 |
|
@oesteban What's the status of this one? |
|
Unless we want to add regression tests (which would be advisable), this would be good to go. |
|
I see we have three tissue probability maps within the test that that could be used here. |
|
Actually, hold on. I'll add the tests. |
in_mask