Skip to content
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

Align with old meica #952

Merged
merged 54 commits into from
Mar 22, 2024
Merged

Conversation

handwerkerd
Copy link
Member

@handwerkerd handwerkerd commented Jun 8, 2023

Closes #929.

While working on the decision tree modularization, we noticed that the tedana decision tree had a difference from the older MEICA decision tree. Components that used to be classified as midK could later be ignored rather than rejected, but in tedana, once they were rejected, they couldn't later be ignored. More details are in #929. This PR should address this issue.

Changes proposed in this pull request:

  • kundu.json was copied to tedana_orig.json
  • meica.json is similar to kundu.json, but was edited to include a provisionalreject intermediate classification. This is what was originally called mid kappa or midk That is used to allow the same components to be evaluated across multiple nodes and eventually rejected. This should match the decision tree steps in MEICA v2.5.
  • Several people compared the results of tedana_orig and meica. While tedana_orig rejects more, everything it rejected seemed reasonable to reject. For now tedana_orig (formerly kundu) will remain the default and this with NOT be labeled a breaking change
  • The [tedana_only/meica].[tex/png] flow charts were updated to reflect the above changes
    • references.bib now points to doi = {10.6084/m9.figshare.25251433.v2} but version 2 does not yet exist. Once others are happy with these new tex/png files I'll upload them to figshare to make version 2.
  • Documentation explaining this change and the relationship between the three decision trees was updated in multiple .rst files and in function doc strings.
  • If someone tries to run tedana using kundu as the three it will log a warning and then run using tedana_only
  • All 3 json files for the decision trees were cleaned up to remove empty log_extra_info fields. When Black was run on these json files, it make additional style changes.
  • The _comment text in tedana_orig.json was also edited to highlight the points where it differs from meica.json

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.68%. Comparing base (9cbd484) to head (6270013).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #952      +/-   ##
==========================================
+ Coverage   89.59%   89.68%   +0.09%     
==========================================
  Files          26       26              
  Lines        3478     3482       +4     
  Branches      614      615       +1     
==========================================
+ Hits         3116     3123       +7     
+ Misses        213      211       -2     
+ Partials      149      148       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@handwerkerd handwerkerd added breaking change WIll make a non-trivial change to outputs TE-dependence issues related to TE dependence metrics and component selection labels Jun 9, 2023
@handwerkerd handwerkerd marked this pull request as ready for review June 15, 2023 14:44
tsalo
tsalo previously approved these changes Aug 15, 2023
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@handwerkerd
Copy link
Member Author

@n-reddy @dowdlelt Do either of you have a chance to run this on a bunch of data during the next few weeks? There will be differences, but I want to make sure the differences are relatively minor.

@n-reddy
Copy link
Collaborator

n-reddy commented Aug 15, 2023

Yes, I can test this. To confirm, you want to compare the kundu tree modified here to the kundu tree that is used in the current version of tedana?

@handwerkerd
Copy link
Member Author

Yes, I can test this. To confirm, you want to compare the kundu tree modified here to the kundu tree that is used in the current version of tedana?

Correct. Compare current kundu tree to the kundu tree used in this PR. Thank you.

@dowdlelt
Copy link
Collaborator

I'll try and jump on this at some point, but might be just a bit... Sounds straightforward enough

@handwerkerd handwerkerd removed the breaking change WIll make a non-trivial change to outputs label Feb 23, 2024
@handwerkerd
Copy link
Member Author

I made all the changes we discussed a few months ago and this should now be ready to review & hopefully merge. I updated the opening comment to detail everything that is or isn't changing there. Of note, I originally called this a breaking change, but, since we decided to keep the same tree as our default, this is no longer a breaking change.

Since this is both some code changes and explanatory documentation, I wouldn't mind a few extra eyes to make sure the documentation is clear. Having a few people run this to make sure nothing surprising happens on real data would also be useful. (@tsalo @eurunuela @dowdlelt @goodalse2019 @n-reddy )

@handwerkerd
Copy link
Member Author

@n-reddy and @dowdlelt Could you look this over and run it with the tedana and meica trees to make sure everything looks good? If there are any issues, I'd like to be able to discuss at the dev call next week.

Copy link
Collaborator

@n-reddy n-reddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on 60 datasets with expected outcomes: for 30 datasets, meica accepted components that tedana_orig rejected. Out of these 30, 8 datasets had high (>20%) variance explained by the changed classification components.

Documentation LGTM!

Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a minor typo fix.

docs/faq.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@handwerkerd
Copy link
Member Author

@n-reddy Can you re-approve after the last few updates?

@handwerkerd handwerkerd merged commit f084dc4 into ME-ICA:main Mar 22, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TE-dependence issues related to TE dependence metrics and component selection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Divergence between older MEICA component selection and tedana
5 participants