Aligning robustICA with current Main +

[handwerkerd]

@BahmanTahayori This PR aligns your robustICA code with the current Main branch of tedana. This will allow for continued development and eventual merging.

I'm also making some more changes to the code to get it closer to ready to merge. I'll keep a running log of what I'm doing here:

• decomposition/ica.py I think the edited code to try DBSCAN and then try AgglomerativeClustering if it failed would actually run both every time. I edited it to only run AgglomerativeClustering if DBSCAN failed.
• decomposition/ica.py used rica as a variable for Robust ICA. We also use rica for https://github.com/ME-ICA/rica I changed the variable name to robust_ica` to avoid confusion
• Changed the default in config.py to fastica. Particularly since this will take longer to run, I don't want to change the default until we've confirmed this is a stable addition.
• Removed testing robustICA from the 3-echo integration test (it was a repeat of the 5-echo integration test and slowed down testing a lot)
• Added a new Q and answer in faq.rst and updated approach.rst and the API and command line help text.

I think I've made all the code changes I wanted to make. This should be ready if @BahmanTahayori thinks it's ok to merge into his version of the code.

[BahmanTahayori]

Thank you very much @handwerkerd for all the updates. I will go through all of it in the next two days and will keep you posted.

[BahmanTahayori]

Dear @Lestropie, this is the recent update that we are working on now.

[Lestropie]

The changes as presented by the PR are not terribly useful for reviewing. The vast majority of content here is a recapitulation of the upstream main changes. Because the changes under discussion are only one commit (8e6878f) it's relatively easy to isolate and assess; but in a more complex scenario it would become impossible to disentangle.

The preferable workflow for code review would be either:

• A PR superseding Adding robustica option to ICA decomposition to achieve consistent results  ME-ICA/tedana#1013, doing ME-ICA:main <- handwerkerd:adding-robustica. From there one should be able to isolate to look at the proposed changes of only those commits responsible for resolving Adding robustica option to ICA decomposition to achieve consistent results  ME-ICA/tedana#1013 against latest main, and assess the PR as a whole.

• A PR targeting https://github.com/BahmanTahayori/tedana_florey/tree/main that includes only modification to the changes adding RobustICA, which make it more compatible with ME-ICA-main once that merge occurs but doesn't actually include that merge (for testing of merge compatibility can do that merge locally on a different branch).

decomposition/ica.py I think the edited code to try DBSCAN and then try AgglomerativeClustering if it failed would actually run both every time. I edited it to only run AgglomerativeClustering if DBSCAN failed.

@BahmanTahayori: Not only would the former code run both every time, but the output from AgglomerativeClustering would always be used, regardless of whether DBSCAN failed. This will be relevant for any results you have generated using that code. Apologies, this is completely my mistake (ME-ICA@979d026), there should have been a break in there; not sure how I managed to muck that up.

[BahmanTahayori]

Thanks @Lestropie for your feedback. Yes, you are right that would always give us the agglomerative result. I had it fixed in a branch that I was working on. I will push it to the main when I finalise the changes.

[Lestropie]

To clarify: while there are different workflows that may have been preferable from the perspective of specifically code review, because the proposed modifications are in this case relatively small I can check them just from that single commit. I would advocate for merging this PR and then shifting any outstanding review processes to ME-ICA#1013. All of the commits listed in this PR originating from ME-ICA/main should disappear in that PR as they would be common to both branches. And the proposed changeset is small enough that it can be re-reviewed from the ME-ICA#1013 PR as a whole.

@BahmanTahayori if you have your own fix for my clustering method selection, that will then conflict with @handwerkerd's fix here if you attempt to merge it in; so let me know if you need help resolving the merge conflict. Also make sure to flag on ME-ICA#1013 that there are other changes you are actively working on for that PR, especially if there are any aspects of it where assistance from development team members might be beneficial.

[handwerkerd]

Thank you @Lestropie and @BahmanTahayori for feedback. If I understand correctly, I agree that, if nothing here seems like a dramatic change, it's best to merge this so that it's easier to directly compare BahmanTahayori:main to ME-ICA:main Then we can have additional reviews and discussions within that PR.

[handwerkerd]

@BahmanTahayori I think I just finished documentation changes so ME-ICA#1013 should be ready for review once this is merged into your branch. I have some time to focus on this now so I'd like to get this moving soon. There are two options:

1. If you have the time, you merge this into BahmanTahayori:main others review and either you make the requested changes or I make the changes and you just have to merge them into your main
2. If you're short on time now, I could open a new PR with a branch I can push onto and I take the lead responding to additional feedback from others. The code you contributed would still be listed under your name.

I'm fine with either option, so let me know your preference.

[Lestropie]

@handwerkerd Having reviewed individual commits, I think the play is to merge this and then re-review the changes as a whole over at ME-ICA#1013. I just left a few suggestions on the docs changes.