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

Made AIC the default maPCA option #849

Merged
merged 6 commits into from
Mar 16, 2022
Merged

Conversation

eurunuela
Copy link
Collaborator

Closes #843 .

Changes proposed in this pull request:

  • Updated all references to the default maPCA option from mdl to aic.
  • Made aic the default maPCA option.
  • Updated three-echo test to use aic.

@eurunuela
Copy link
Collaborator Author

I think this PR will fix the failing three-echo test in #839.

@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #849 (b28a282) into main (eaa0920) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #849      +/-   ##
==========================================
- Coverage   93.27%   93.09%   -0.19%     
==========================================
  Files          27       27              
  Lines        2217     2217              
==========================================
- Hits         2068     2064       -4     
- Misses        149      153       +4     
Impacted Files Coverage Δ
tedana/workflows/tedana.py 89.92% <ø> (ø)
tedana/decomposition/pca.py 86.51% <100.00%> (ø)
tedana/decomposition/ica.py 82.75% <0.00%> (-13.80%) ⬇️

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 eaa0920...b28a282. Read the comment docs.

@tsalo
Copy link
Member

tsalo commented Feb 22, 2022

Thanks for this. One thing I would like to see added to our documentation is a very short explanation of why we use AIC by default. Something like...

We have chosen AIC as the default PCA criterion because it tends to result in fewer components than the Kundu methods, which increases the likelihood that the ICA step will successfully converge, but also, in our experience, retains enough components for meaningful interpretation later on.

I doesn't have to be that, but something to that effect would be nice. I also think this probably deserves a breaking-change label.

@eurunuela eurunuela added the breaking change WIll make a non-trivial change to outputs label Feb 23, 2022
@eurunuela
Copy link
Collaborator Author

Thanks for this. One thing I would like to see added to our documentation is a very short explanation of why we use AIC by default. Something like...

We have chosen AIC as the default PCA criterion because it tends to result in fewer components than the Kundu methods, which increases the likelihood that the ICA step will successfully converge, but also, in our experience, retains enough components for meaningful interpretation later on.

Let me know what you think of the changes added in c610667.

I doesn't have to be that, but something to that effect would be nice. I also think this probably deserves a breaking-change label.

Done!

@eurunuela eurunuela requested a review from tsalo February 23, 2022 16:26
@handwerkerd
Copy link
Member

I'll try to look at this more carefully soon, but one thing I've noticed while running tedana is that there's no logging for various pca options. When the default option is used, I'm not sure anywhere in the log will say what the default option is. I think we should add some logging to places like this

@eurunuela
Copy link
Collaborator Author

I'll try to look at this more carefully soon, but one thing I've noticed while running tedana is that there's no logging for various pca options. When the default option is used, I'm not sure anywhere in the log will say what the default option is. I think we should add some logging to places like this

The logging happens here. The only thing that could be missing in my opinion is the maPCA criteria.

@tsalo tsalo added the decomposition issues related to decomposition methods label Mar 10, 2022
Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

I expanded one comment a bit.
I can't figure out how to suggest a change in an area where you didn't make any edits, but I'd replace
https://github.com/eurunuela/tedana/blob/c61066788779da683d6246dc905b326a74d0bd93/tedana/decomposition/pca.py#L228
with
LGR.info(f"Computing PCA of optimally combined multi-echo data with selection criteria: {algorithm}")

That will have slightly ugly parsing if someone enters a number for algorithm, but, if someone is entering a number, they'll hopefully understand what they're doing.

Otherwise, this all looks good to me.

docs/approach.rst Outdated Show resolved Hide resolved
eurunuela and others added 2 commits March 16, 2022 10:54
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
@eurunuela
Copy link
Collaborator Author

Thank you for your feedback @handwerkerd, b28a282 should address your comments.

Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @eurunuela !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change WIll make a non-trivial change to outputs decomposition issues related to decomposition methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change default maPCA option to AIC
4 participants