[FIX] Do not use minimum mask for OC data in tedpca #204
Conversation
Codecov Report
@@ Coverage Diff @@
## master #204 +/- ##
==========================================
- Coverage 51.42% 51.39% -0.03%
==========================================
Files 32 32
Lines 1964 1965 +1
==========================================
Hits 1010 1010
- Misses 954 955 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #204 +/- ##
=======================================
Coverage 51.42% 51.42%
=======================================
Files 32 32
Lines 1964 1964
=======================================
Hits 1010 1010
Misses 954 954
Continue to review full report at Codecov.
|
| @@ -266,13 +266,13 @@ def tedpca(catd, OCcatd, combmode, mask, t2s, t2sG, | |||
|
|
|||
| if len(ste) == 1 and ste[0] == -1: | |||
| LGR.info('Computing PCA of optimally combined multi-echo data') | |||
| d = OCcatd[utils.make_min_mask(OCcatd[:, np.newaxis, :])][:, np.newaxis, :] | |||
| d = OCcatd[mask, :][:, np.newaxis, :] | |||
There was a problem hiding this comment.
One more question: for the two other options, we explicitly cast to float64. Are we sure OCcatd is float64 already ?
There was a problem hiding this comment.
I don't see anywhere where the type for the optimally combined data is explicitly set. Honestly, I think casting to float64 is unnecessary for any of the data, but we can do it to the optimally combined data too just to be consistent.
There was a problem hiding this comment.
I'm fine with either path forward, I'm just pro-consistency unless otherwise specified 😅
There was a problem hiding this comment.
Maybe we can open this as another issue, because I'm not sure how much it overlaps here, but I'm actually pretty sure your initial take (that casting to float64 is unnecessary) is correct. My reason for bringing it up now is because it's an easy chance to change it and I do think we're increasing memory usage with the re-casting....
|
@jbteves or @dowdlelt: if you run this branch on the data you mentioned in in #168 (comment) with |
|
@emdupre kicking off a test run now. Hope to have an answer soon. |
|
@dowdlelt are you running this on our shared server? If so I don't see a point in replicating efforts, if not I'll run a test on it so we have at least two OS's. |
|
negative @jbteves and I managed to lock my local machine because I got greedy with resources. Spin it up on the shared server, and I'll try another run tomorrow am. |
|
Okay, @emdupre @tsalo here is how I tested:
From this I got all 0's in the resulting subtraction, which indicates the results should be the same, for the two runs that have already finished (still waiting on several). Let me know if you want additional tests done-- I can try to knock them out. If you catch me in the next half hour I'll run them overnight. |
|
That's reassuring ! Just to make sure I understand: the matrix you re-shaped was which output file ? Thanks so much for doing that !! ✨ |
|
|
|
Great ! The only other way I can think to test it would be to directly compare the files, with something like import numpy as np
import nibabel as nb
file1 = nb.load('filename1.nii').get_data()
file2 = nb.load('filename2.nii').get_data()
np.allclose(file1, file2)But that should give you just a Unless @tsalo has any other concerns I'll go ahead and get this merged tomorrow ! |
|
Sounds good to me. I also think that, since this fix has the potential to impact a fair amount of data, we should probably draft a minor release after it's merged. Well this PR and the other things we've managed to do since the last release (e.g., tedort, the tedpca split, changes to the CLI, and vast improvements to the documentation). |
jbteves
added
breaking change
Closes #168.
Changes proposed in this pull request: