[REF] Modularize metric calculation

[tsalo]

Closes #501.

Changes proposed in this pull request:

• Modularize metrics.
• Add while loop to cluster-extent thresholding to maximize similarity in number of significant voxels between comparison maps.
• Add signal-noise_z metric, which should ultimately replace signal-noise_t.

[codecov]

Codecov Report

Merging #591 (c1f80cc) into main (ad664ae) will decrease coverage by 0.53%.
The diff coverage is 91.39%.

@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
- Coverage   92.81%   92.27%   -0.54%     
==========================================
  Files          25       27       +2     
  Lines        1879     2137     +258     
==========================================
+ Hits         1744     1972     +228     
- Misses        135      165      +30     

Impacted Files	Coverage Δ
tedana/reporting/dynamic_figures.py	100.00% <ø> (ø)
tedana/workflows/tedana.py	90.30% <66.66%> (+0.46%)	⬆️
tedana/metrics/_utils.py	73.77% <73.77%> (ø)
tedana/utils.py	97.50% <85.71%> (-0.80%)	⬇️
tedana/selection/tedica.py	92.56% <88.88%> (-0.81%)	⬇️
tedana/metrics/dependence.py	93.25% <93.25%> (ø)
tedana/metrics/collect.py	95.10% <95.10%> (ø)
tedana/stats.py	96.62% <96.42%> (-0.10%)	⬇️
tedana/decay.py	97.24% <100.00%> (+0.07%)	⬆️
tedana/decomposition/pca.py	86.66% <100.00%> (ø)
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad664ae...c1f80cc. Read the comment docs.

[tsalo]

The only failing tests are CodeCov, because of a 0.73% decrease in coverage. So that's good at least!

[tsalo]

Update from today's call: Will everyone please try to both review this and test it out on your own code. Also, @jbteves and @handwerkerd have committed to reviewing.

[jbteves]

Hey @tsalo, thanks so much for this, this is awesome!
While I was looking through this one of the concerns I have is that we'll have to maintain the order of metric calculation and checks for metrics in the core part of the code. I think this could be difficult to maintain, especially if our plan is to add or delete metrics frequently. I know that we're generally opposed to using object-oriented approaches because they can be conceptually difficult, but this is a case where I think it will actually be easier to follow if we use classes. I came up with something along these lines:

• A metric superclass contains a class-specific table telling you which metrics are calculated and if so, what their values are
• Each individual class inherits from metric superclass (easy template to follow), and has a constructor that checks to see if that metric's value exists in the table; if not, calculate its dependencies and then it
• This allows you to do recursion to figure out dependencies, simplifying the core code to just specifying the top-level metrics you want
• Modify [REF] Decision tree modularization #592 to iterate over the needed metrics and calculate them.
• Would have to wipe out the metric values at the end of each component to avoid memory explosion, but that's easy to implement by simply pointing the metric to None, so the GC takes out the big variables
• Maybe add a verbosity function which saves intermediates? Currently we're not saving some of these and it might be good to do so.

I would propose to do the work stated above if we decide it's a good idea. I'm open to other suggestions, but I am a little concerned about having to edit core code in order to add metrics, if that makes sense.

[jbteves]

@tsalo it looks like we end up losing several metric files. I talked to @handwerkerd and he thinks you chatted about that some time ago. Do you recall if you decided that it was okay to lose those metric files?
What would you think about adding an option to save used metrics for later review?
Lastly, would it be okay to push a few commits to this branch to bring it in line with master?

[tsalo]

Sorry, what metric files?

Also yes, feel free to push changes.

[jbteves]

Sorry for the ambiguity. In this branch, we're no longer writing the following:

[tsalo]

Ah, I see. I believe all of those were only used for the figures in our documentation, but we can figure out how to output them again.

[jbteves]

I don't see a need to do that right away. If we miss them we can bring them back. In the future, though, we should probably create an option to get metric maps.

[jbteves]

Alright, this is mostly in line with main. There is basically one hack that I had to make where we save the verbose outputs every time we collect the metrics, because the verbose outputs require a lot of information that doesn't live outside of that function, unlike the metrics which don't require echo-specific information after that step. This actually slows down the four-echo test a bit since that means each re-run requires a series of file writes. I can't think of a much better way of doing things without further modularization. However, this branch easily gets out of sync with main and I think it's better to get it in, since it touches so many parts of the code.
It additionally contains some modifications to RTD because for some reason our old configuration just stopped working. I copied the gist linked there into a javascript file to more closely match the setup specification on their documentation.
Additionally, unlike main I generate metric metadata from the component table rather than during metric creation because that ended up being a simpler way to handle the metadata while I was fixing the merge conflicts.

[jbteves]

@tsalo and @handwerkerd do you mind taking a look?
Anyone else is welcome, but this is a very dense PR!

[eurunuela]

I have just compared the results with main and this PR on a motor task and a resting-state dataset. I used np.allclose() to check if all metrics were identical, and they were. I ran tedana with all the default options by the way.

[handwerkerd]

Looks ready to merge to me! I've looked through the code for clarity and run it on several distinct data sets and the results don't change between this PR & main!

[handwerkerd]

Suggested change

	"Classification from the manual classification procedure."
	"Classification from the classification procedure."

The word manual was added here. Since this is the descriptor for any classification, I think saying it's just for the manual classification is incorrect.

[handwerkerd]

@emdupre & @eurunuela, Any chance one of you can give this a look-over so that we can approve this week? It would be really nice to just say this is merged, without any qualifiers, on the OHBM poster.

We could have me and @jbteves formally approve and @tsalo merge, but it would be nice to have a few more eyes. As far as I can tell, the resulting metric calculations are effectively unchanged. I've tested it on a few data sets with different voxel sizes, run lengths, and 3 vs 5 echoes. During my review, I saw some things that might be worth revisiting later, but were functional as is.

Please let us know if we should wait for your reviews or if you're ok with us merging.

[eurunuela]

@emdupre & @eurunuela, Any chance one of you can give this a look-over so that we can approve this week? It would be really nice to just say this is merged, without any qualifiers, on the OHBM poster.

I'll review it before Saturday.

[emdupre]

I'll also aim to review this weekend, but if you have the necessary reviews don't feel compelled to wait on me !

[notZaki]

I compared this PR vs main on the 88 subjects in the Cambridge collection, using both tedpca='mdl' and tedpca='kundu'.
With mdl, the denoised data was exactly identical. With kundu, there were minor differences but they were all below 1e-7 in all cases.

The pca_metrics.tsv file has more detail in the PR version. 👍
Edit: On the flip side, PR is slightly slower and took ~20 minutes longer to process the 88 subjects with kundu. The time difference was smaller with mdl.

[eurunuela]

I'm happy with the changes. This is some excellent work you've done @jbteves @handwerkerd , thanks!

I only have very minor comments that can be addressed after this PR is merged.

[eurunuela]

We should add a couple of unit tests to check all if statements here. I'd rather do it once we merge this PR.

[eurunuela]

How about a LGR.debug here?

[eurunuela]

Same as before, I think we need to make sure these errors are raised.

[eurunuela]

Wouldn't it be more elegant to have this dictionary on a JSON file? I think it would make adding new metrics easier.

[eurunuela]

Same thing as before, we should add tests for the if statements.

[eurunuela]

I think the wording in this sentence is a little complicated to follow.

[eurunuela]

Wording

[jbteves]

@eurunuela please also credit @tsalo as mostly we were adapting his branch.

@handwerkerd any strong feelings about addressing these changes now vs. later?

[eurunuela]

@eurunuela please also credit @tsalo as mostly we were adapting his branch.

True! I had completely forgotten! Sorry about that @tsalo and thanks for your amazing work!

[jbteves]

Since this is text I just want to clarify that I wasn't being short with you, I just want to make sure he gets due credit! It's a significant change that was very intricate.

[eurunuela]

Since this is text I just want to clarify that I wasn't being short with you, I just want to make sure he gets due credit! It's a significant change that was very intricate.

Oh, don't worry! I'm actually grateful that you pointed it out :)

[handwerkerd]

These look like good changes, but they won't affect functionality and should be easy to do in a later PR. My preference would be to merge before Monday and open a new issue with these suggested changes.
If @emdupre might look at this over the weekend, I'd wait for her review. Otherwise, @tsalo, you want the honor of merging Sunday night?

[emdupre]

We should alert the user why this would happen with the LGR.

[emdupre]

LGTM ! I would like to see a few more tests, but we can merge and then add those next ! 🚀

[handwerkerd]

@tsalo You willing to squash & merge today? We can then open another issue or two to start addressing all the comments here.

[tsalo]

I'll do it now then. Thanks.