-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add --ctab
parameter to ica_reclassify
#1184
Conversation
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.
LGTM. I do think we should update the docs before merging this PR in.
Is ica_reclassify documented in detail outside of the argparse-based documentation in https://tedana.readthedocs.io/en/stable/usage.html#running-the-ica-reclassify-workflow? |
Ironically, it looks like the documentation on using RICA is outdated and actually matches what this PR proposes:
|
You may be right. I think that may be the only documentation we have. For some reason I thought I remembered a different section. |
Sorry I didn't notice the open issue a few days ago before you started to work on the PR. I do have some concerns about this.
tedana/tedana/workflows/ica_reclassify.py Line 430 in 897851e
I have two alternate proposals to address the above:
Thoughts? |
Adding more details to my proposal 2 above:
I'm including a lot of details, because I wanted to specify everything, but most of the mechanics for the above already exists in the code (plus adding ctab that's already in this PR). The only non-trivial new thing would be a function to merge columns in two component tables that checks for problems. If you decide to follow this approach, I can help/contribute. |
I hadn't considered that this PR would ignore the status table from the previous run. For my use case, I don't need that status table- it's still available in the original tedana directory, and I'm not planning to delete that. I just want to apply a component table + mixing matrix to my data to get an updated orthogonalized mixing matrix that I can use for later denoising. It would be nice to have some of the other bells and whistles (esp. an HTML report with component plots), but that's not a dealbreaker. It might just be easier for me to write a short script to handle this for my data.
AROMA does include its own metrics and has multiple classification tags that may apply to each component, so I think this would end up being cluttered at the command line and would lose some information.
I think this is a solid idea. I suppose, with that approach, one could provide multiple component tables, and even have an option for the decision strategy (e.g., reject if any component table rejects, accept if any component table accepts, or apply a rejection threshold based on the percentage of component tables that reject the component). |
If that's your main goal, then I'd recommend just sending the new list of components to reject to tedana and adding functionality to have an You'll still have the component table you created, but it wouldn't mess with how the component table created by tedana interacts with other parts. If you want a fully combined component table, then we should do something slightly more expansive that merges the two component tables within tedana. [update] With this option, your workflow would be:
|
We discussed this a bit on Mattermost, so I'll try to summarize our conversation. For most users, allowing Basically, I'll close this PR in favor of a followup PR from @handwerkerd that adds classification tag parameters, and I'll use custom code for my AROMA+tedana denoising. |
I think your custom code could still be useful for others in the community. Maybe we could add a link to it in our docs if you think that's a good idea. |
Closes #1182.
Changes proposed in this pull request:
--ctab
parameter toica_reclassify
that accepts a component table.--manacc
,--manrej
, and--ctab
are not provided, then raise an error (was being raised if--manacc
and--manrej
weren't provided).