-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding robustica option to ICA decomposition to achieve consistent results (new PR) #1125
Conversation
Manual modification of commit f2cdb4e to remove unwanted file additions.
Cherry-pick of 41354cb.
From: BahmanTahayori#2. Co-authored-by: Robert E. Smith <[email protected]>
Cherry-pick of da1b128 (with modification).
Addition of RobustICA (after RS clean)
Co-authored-by: Dan Handwerker <[email protected]>
Co-authored-by: Dan Handwerker <[email protected]>
* RobustICA: Restructure code loop over robust methods * Addressing the issue with try/except --------- Co-authored-by: Bahman <[email protected]>
In this commit, some of the comments from Daniel Handwerker and Robert Smith were incorporated.
* Fixing the problem of argument parser for n_robust_runs. * Removing unnecessary tests from the test_integration. There are 3 tests for echo as before, but the ica_method is robustica for five and three echos and fatsica for the four echo test.
Conflicts: tedana/workflows/tedana.py
Co-authored-by: Robert Smith <[email protected]>
Co-authored-by: Robert Smith <[email protected]>
* Limit current adaptive mask method to brain mask (ME-ICA#1060) * Limit adaptive mask calculation to brain mask. Limit adaptive mask calculation to brain mask. Expand on logic of first adaptive mask method. Update tedana/utils.py Improve docstring. Update test. Add decreasing-signal-based adaptive mask. Keep removing. Co-Authored-By: Dan Handwerker <[email protected]> * Use `compute_epi_mask` in t2smap workflow. * Try fixing the tests. * Fix make_adaptive_mask. * Update test_utils.py * Update test_utils.py * Improve docstring. * Update utils.py * Update test_utils.py * Revert "Update test_utils.py" This reverts commit 259b002. * Don't take absolute value of echo means. * Log echo-wise thresholds in adaptive mask. * Add comment about non-zero voxels. * Update utils.py * Update test_utils.py * Update test_utils.py * Update test_utils.py * Log the thresholds again. * Address review. * Fix test. --------- Co-authored-by: Dan Handwerker <[email protected]> * Update nilearn requirement from <=0.10.3,>=0.7 to >=0.7,<=0.10.4 (ME-ICA#1077) * Add adaptive mask plot to report (ME-ICA#1073) * Update scikit-learn requirement (ME-ICA#1075) Updates the requirements on [scikit-learn](https://github.com/scikit-learn/scikit-learn) to permit the latest version. - [Release notes](https://github.com/scikit-learn/scikit-learn/releases) - [Commits](scikit-learn/scikit-learn@0.21.0...1.4.2) --- updated-dependencies: - dependency-name: scikit-learn dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update pandas requirement from <=2.2.1,>=2.0 to >=2.0,<=2.2.2 (ME-ICA#1076) Updates the requirements on [pandas](https://github.com/pandas-dev/pandas) to permit the latest version. - [Release notes](https://github.com/pandas-dev/pandas/releases) - [Commits](pandas-dev/pandas@v2.0.0...v2.2.2) --- updated-dependencies: - dependency-name: pandas dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update bokeh requirement from <=3.4.0,>=1.0.0 to >=1.0.0,<=3.4.1 (ME-ICA#1078) Updates the requirements on [bokeh](https://github.com/bokeh/bokeh) to permit the latest version. - [Changelog](https://github.com/bokeh/bokeh/blob/branch-3.5/docs/CHANGELOG) - [Commits](bokeh/bokeh@1.0.0...3.4.1) --- updated-dependencies: - dependency-name: bokeh dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Load user-defined mask as expected by plot_adaptive_mask (ME-ICA#1079) * DOC desc-optcomDenoised -> desc-denoised (ME-ICA#1080) * docs: add mvdoc as a contributor for code, bug, and doc (ME-ICA#1082) * docs: update README.md * docs: update .all-contributorsrc --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> * Identify the last good echo in adaptive mask instead of sum of good echoes (ME-ICA#1061) * Limit adaptive mask calculation to brain mask. Limit adaptive mask calculation to brain mask. Expand on logic of first adaptive mask method. Update tedana/utils.py Improve docstring. Update test. Add decreasing-signal-based adaptive mask. Keep removing. Co-Authored-By: Dan Handwerker <[email protected]> * Use `compute_epi_mask` in t2smap workflow. * Try fixing the tests. * Fix make_adaptive_mask. * Update test_utils.py * Update test_utils.py * Improve docstring. * Identify the last good echo instead of sum. Improve docstring. Update test_utils.py Update test_utils.py Fix make_adaptive_mask. Try fixing the tests. Use `compute_epi_mask` in t2smap workflow. Limit adaptive mask calculation to brain mask. Limit adaptive mask calculation to brain mask. Expand on logic of first adaptive mask method. Update tedana/utils.py Improve docstring. Update test. Add decreasing-signal-based adaptive mask. Keep removing. Co-Authored-By: Dan Handwerker <[email protected]> * Fix. * Update utils.py * Update utils.py * Try fixing. * Update utils.py * Update utils.py * add checks * Just loop over voxels. * Update utils.py * Update utils.py * Update test_utils.py * Revert "Update test_utils.py" This reverts commit 259b002. * Update test_utils.py * Update test_utils.py * Remove checks. * Don't take absolute value of echo means. * Log echo-wise thresholds in adaptive mask. * Add comment about non-zero voxels. * Update utils.py * Update utils.py * Update test_utils.py * Update test_utils.py * Update test_utils.py * Log the thresholds again. * Update test_utils.py * Update test_utils.py * Update test_utils.py * Add simulated data to adaptive mask test. * Clean up the tests. * Add value that tests the base mask. * Remove print in test. * Update tedana/utils.py Co-authored-by: Dan Handwerker <[email protected]> * Update tedana/utils.py Co-authored-by: Dan Handwerker <[email protected]> --------- Co-authored-by: Dan Handwerker <[email protected]> * Output RMSE map and time series for decay model fit (ME-ICA#1044) * Draft function to calculate decay model fit. * Calculate root mean squared error instead. * Incorporate metrics. * Output RMSE results. * Output results in tedana. * Hopefully fix things. * Update decay.py * Try improving performance. * Update decay.py * Fix again. * Use tqdm. * Update decay.py * Update decay.py * Update decay.py * Update expected outputs. * Add figures. * Update outputs. * Include global signal in confounds file. * Update fiu_four_echo_outputs.txt * Rename function. * Rename function. * Update tedana.py * Update tedana/decay.py Co-authored-by: Dan Handwerker <[email protected]> * Update decay.py * Update decay.py * Whoops. * Apply suggestions from code review Co-authored-by: Dan Handwerker <[email protected]> * Fix things maybe. * Fix things. * Update decay.py * Remove any files that are built through appending. * Update outputs. * Add section on plots to docs. * Fix the description. * Update docs/outputs.rst Co-authored-by: Dan Handwerker <[email protected]> * Update docs/outputs.rst * Fix docstring. --------- Co-authored-by: Dan Handwerker <[email protected]> * minimum nilearn 0.10.3 (ME-ICA#1094) * Use nearest-neighbors interpolation in `plot_component` (ME-ICA#1098) * Use nearest-neighbors interpolation in plot_stat_map. * Only use NN interp for component maps. * Update scipy requirement from <=1.13.0,>=1.2.0 to >=1.2.0,<=1.13.1 (ME-ICA#1100) Updates the requirements on [scipy](https://github.com/scipy/scipy) to permit the latest version. - [Release notes](https://github.com/scipy/scipy/releases) - [Commits](scipy/scipy@v1.2.0...v1.13.1) --- updated-dependencies: - dependency-name: scipy dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update scikit-learn requirement from <=1.4.2,>=0.21 to >=0.21,<=1.5.0 (ME-ICA#1101) Updates the requirements on [scikit-learn](https://github.com/scikit-learn/scikit-learn) to permit the latest version. - [Release notes](https://github.com/scikit-learn/scikit-learn/releases) - [Commits](scikit-learn/scikit-learn@0.21.0...1.5.0) --- updated-dependencies: - dependency-name: scikit-learn dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update numpy requirement from <=1.26.4,>=1.16 to >=1.16,<=2.0.0 (ME-ICA#1104) * Update numpy requirement from <=1.26.4,>=1.16 to >=1.16,<=2.0.0 Updates the requirements on [numpy](https://github.com/numpy/numpy) to permit the latest version. - [Release notes](https://github.com/numpy/numpy/releases) - [Changelog](https://github.com/numpy/numpy/blob/main/doc/RELEASE_WALKTHROUGH.rst) - [Commits](numpy/numpy@v1.16.0...v2.0.0) --- updated-dependencies: - dependency-name: numpy dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * Use np.nan instead of np.NaN --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Salo <[email protected]> * Filter out non-diagonal affine warning (ME-ICA#1103) * Filter out non-diagonal affine warning. * Fix warning capture. * Update tedana/reporting/static_figures.py Co-authored-by: Dan Handwerker <[email protected]> * Update static_figures.py --------- Co-authored-by: Dan Handwerker <[email protected]> * Update bokeh requirement from <=3.4.1,>=1.0.0 to <=3.5.0,>=3.5.0 (ME-ICA#1109) * Update bokeh requirement from <=3.4.1,>=1.0.0 to <=3.5.0,>=3.5.0 Updates the requirements on [bokeh](https://github.com/bokeh/bokeh) to permit the latest version. - [Changelog](https://github.com/bokeh/bokeh/blob/branch-3.6/docs/CHANGELOG) - [Commits](bokeh/bokeh@1.0.0...3.5.0) --- updated-dependencies: - dependency-name: bokeh dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * Update pyproject.toml --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Salo <[email protected]> * Update scikit-learn requirement from <=1.5.0,>=0.21 to <=1.5.1,>=1.5.1 (ME-ICA#1108) * Update scikit-learn requirement from <=1.5.0,>=0.21 to <=1.5.1,>=1.5.1 Updates the requirements on [scikit-learn](https://github.com/scikit-learn/scikit-learn) to permit the latest version. - [Release notes](https://github.com/scikit-learn/scikit-learn/releases) - [Commits](scikit-learn/scikit-learn@0.21.0...1.5.1) --- updated-dependencies: - dependency-name: scikit-learn dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * Update pyproject.toml to restore minimum version of scikit-learn --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Dan Handwerker <[email protected]> * Update scipy requirement from <=1.13.1,>=1.2.0 to <=1.14.0,>=1.14.0 (ME-ICA#1106) * Update scipy requirement from <=1.13.1,>=1.2.0 to <=1.14.0,>=1.14.0 Updates the requirements on [scipy](https://github.com/scipy/scipy) to permit the latest version. - [Release notes](https://github.com/scipy/scipy/releases) - [Commits](scipy/scipy@v1.2.0...v1.14.0) --- updated-dependencies: - dependency-name: scipy dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * Update pyproject.toml to retain minimum version of scipy --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Dan Handwerker <[email protected]> Co-authored-by: Eneko Uruñuela <[email protected]> * Update numpy requirement from <=2.0.0,>=1.16 to >=1.16,<=2.0.1 (ME-ICA#1112) Updates the requirements on [numpy](https://github.com/numpy/numpy) to permit the latest version. - [Release notes](https://github.com/numpy/numpy/releases) - [Changelog](https://github.com/numpy/numpy/blob/main/doc/RELEASE_WALKTHROUGH.rst) - [Commits](numpy/numpy@v1.16.0...v2.0.1) --- updated-dependencies: - dependency-name: numpy dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Cleaning up installation instructions (ME-ICA#1113) * install instructions * Update docs/installation.rst Co-authored-by: Taylor Salo <[email protected]> * Update docs/installation.rst Co-authored-by: Eneko Uruñuela <[email protected]> --------- Co-authored-by: Taylor Salo <[email protected]> Co-authored-by: Eneko Uruñuela <[email protected]> * Update bokeh requirement from <=3.5.0,>=1.0.0 to >=1.0.0,<=3.5.1 (ME-ICA#1116) Updates the requirements on [bokeh](https://github.com/bokeh/bokeh) to permit the latest version. - [Changelog](https://github.com/bokeh/bokeh/blob/3.5.1/docs/CHANGELOG) - [Commits](bokeh/bokeh@1.0.0...3.5.1) --- updated-dependencies: - dependency-name: bokeh dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update list of multi-echo datasets (ME-ICA#1115) * Generate metrics from external regressors using F stats (ME-ICA#1064) * Get required metrics from decision tree. * Continue changes. * More updates. * Store necessary_metrics as a list. * Update selection_nodes.py * Update selection_utils.py * Update across the package. * Keep updating. * Update tedana.py * Add extra metrics to list. * Update ica_reclassify.py * Draft metric-based regressor correlations. * Fix typo. * Work on trees. * Expand regular expressions in trees. * Fix up the expansion. * Really fix it though. * Fix style issue. * Added external regress integration test * Got intregration test with external regressors working * Added F tests and options * added corr_no_detrend.json * updated names and reporting * Run black. * Address style issues. * Try fixing test bugs. * Update test_component_selector.py * Update component_selector.py * Use component table directly in selectcomps2use. * Fix. * Include generated metrics in necessary metrics. * Update component_selector.py * responding to feedback from tsalo * Update component_selector.py * Update test_component_selector.py * fixed some testing failures * fixed test_check_null_succeeds * fixed ica_reclassify bug and selector_properties test * ComponentSelector initialized before loading data * fixed docstrings * updated building decision tree docs * using external regressors and most tests passing * removed corr added tasks * fit_model moved to stats * removed and cleaned up external_regressors_config option * Added task regressors and some tests. Now alll in decision tree * cleaning up decision tree json files * removed mot12_csf.json changed task to signal * fixed tests with task_keep signal * Update tedana/metrics/external.py Co-authored-by: Taylor Salo <[email protected]> * Update tedana/metrics/_utils.py Co-authored-by: Taylor Salo <[email protected]> * Update tedana/metrics/collect.py Co-authored-by: Taylor Salo <[email protected]> * Update tedana/metrics/external.py Co-authored-by: Taylor Salo <[email protected]> * Update tedana/metrics/external.py Co-authored-by: Taylor Salo <[email protected]> * Responding to review comments * reworded docstring * Added type hints to external.py * fixed external.py type hints * type hints to _utils collect and component_selector * type hints and doc improvements in selection_utils * no expand_node recursion * removed expand_nodes expand_node expand_dict * docstring lines break on punctuation * updating external tests and docs * moved test data downloading to tests.utils.py and started test for fit_regressors * fixed bug where task regressors retained in partial models * matched testing external regressors to included mixing and fixed bugs * Made single function for detrending regressors * added tests for external fit_regressors and fix_mixing_to_regressors * Full tests in test_external_metrics.py * adding tests * fixed extern regress validation warnings and added tests * sorting set values for test outputs * added to test_metrics * Added docs to building_decision_trees.rst * Added motion task decision tree flow chart * made recommended change to external_regressor_config * Finished documentation and renamed demo decision trees * added link to example external regressors tsv file * Apply suggestions from code review Fixed nuissance typos Co-authored-by: Taylor Salo <[email protected]> * Minor documentation edits --------- Co-authored-by: Taylor Salo <[email protected]> Co-authored-by: Taylor Salo <[email protected]> Co-authored-by: Neha Reddy <[email protected]> * Link to the open-multi-echo-data website (ME-ICA#1117) * Update multi-echo.rst * Update multi-echo.rst * Refactor `metrics.dependence` module (ME-ICA#1088) * Add type hints to metric functions. * Use keyword arguments. * Update tests. * Update dependence.py * Update collect.py * Fix other stuff. * documentation and resource updates (ME-ICA#1114) * documentation and resource updates * Fixed citation numbering and updated posters --------- Co-authored-by: Neha Reddy <[email protected]> * Adding already requested changes * fixed failing tests * updated documentation in faq.rst * more documentation changes --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Taylor Salo <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Matteo Visconti di Oleggio Castello <[email protected]> Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> Co-authored-by: Eneko Uruñuela <[email protected]> Co-authored-by: Taylor Salo <[email protected]> Co-authored-by: Taylor Salo <[email protected]> Co-authored-by: Neha Reddy <[email protected]>
Generating a task list to make sure nothing from #1013 gets missed:
Looking through the code there's nothing setting off alarm bells to me, and the commit history looks appropriate. |
The tests are passing locally but failing here. The one thing I see that is clearly a problem is |
@@ -30,8 +30,9 @@ dependencies = [ | |||
"pandas>=2.0,<=2.2.2", | |||
"pybtex", | |||
"pybtex-apa-style", | |||
"robustica>=0.1.3", | |||
"robustica>=0.1.3, <=0.1.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you pin the version?
"robustica>=0.1.3, <=0.1.3", | |
"robustica==0.1.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work with the version update bot? My assumption was we needed a <=
to give the bot something to update, but I don't really know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependabot should just bump the version. So if robustica releases 0.1.4, we'd get a PR changing it to robustica==0.1.4
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that what we want or would "robustica>=0.1.3, <=0.1.4", be more in line with our other modules? I'm mostly neutral on this.
Hey @handwerkerd, I am getting the same error running the tests in a new environment. So I don't think it's a CircleCI issue. |
Thanks! I think I found the root issue. When I created a clean environment, pip installed
As for next steps, from my understanding, the most robust solution is to ask the makers of [Update: I just cloned robustica and tried a local install with |
I hope they are still around and can fix it, but the lack of commits and PRs since October last year doesn't sound very promising. |
The author responded immediately, but I was looking around and it seems like one of their dependencies, |
Thanks @handwerkerd and the team. I will go through changes and will add a few other things as well. Hopefully, the issue with |
I've been sitting on this for a few days, but I figured I should share. robustica is a single person's project. scikit-learn-extra is an unmaintained library with a few people keeping half-an-eye on it. i want this merged, but adding these as required dependencies might end up being more work for us longer term. We should discuss here or at our Sept 19th (20th in Australia) dev call. The options I see are:
Thoughts? |
Thanks @handwerkerd for your suggestions. Removing the |
@BahmanTahayori fixed the original PR and has a potential solution to the scikit-learn-extra problem so I'm closing this PR and we can continue work in the original PR #1013 |
#1013 was mis-aligned with
main
and there were some issues with getting it aligned again, so I'm opening a new PR that has multiple updates and should be aligned withmain
Overview from that PR: Given that the
FastICA
method depends on its seed value, in some cases the generated output can be substantially different. To overcome this issue, we addedrobustica
(https://github.com/CRG-CNAG/robustica/tree/main/robustica) which is a python library based on Icasso as an option to the ICA decomposition.robustica
clusters many iterations ofFastICA
to achieve consistent results.Changes proposed in this pull request:
ica_method
is added as an option (robustica or fastica) and the user can select between the two methods. The number of runs for when robustica is used can be determined by the user through settingn_robust_runs
. The default is set to `robustica' with 30 number of robust runs. This default value is selected based on our experience using robustica for two different datasets with over 250 subjects' data.fastica
We should re-evaluate after more users start usingrobustica
and provide feedbackfaq.rst
andapproach.rst
Note:
#1013 includes key discussions related to this PR including an evaluation of how the initial number of components requested affects the number of stable components found and an observation of similar ICA component spatial maps with different time series that shouldn't get lost with the switch to a new PR