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

Revisit component sorting within metric calculation #564

Closed
tsalo opened this issue May 1, 2020 · 8 comments · Fixed by #741
Closed

Revisit component sorting within metric calculation #564

tsalo opened this issue May 1, 2020 · 8 comments · Fixed by #741
Labels
refactoring issues proposing/requesting changes to the code which do not impact behavior TE-dependence issues related to TE dependence metrics and component selection

Comments

@tsalo
Copy link
Member

tsalo commented May 1, 2020

Summary

In order to support multiple component selection methods performed on a single decomposition (e.g., running tedana and AROMA on the same ICA results) within fMRIPrep, we should not change the mixing matrix at all by resorting or flipping components.

Next Steps

  1. Remove sorting steps from metric calculation functions.
  2. If any steps in the metric calculation functions require components sorted by kappa, perform the sorting within the step and update the unsorted component table.
  3. Remove reindex argument from dependence_metrics().
  4. Update figures and reports to maximize their utility without sorting the actual component order.
@tsalo tsalo added discussion issues that still need to be discussed refactoring issues proposing/requesting changes to the code which do not impact behavior labels May 1, 2020
@tsalo tsalo changed the title Revisit component sorting within component selection Revisit component sorting within metric calculation May 1, 2020
@tsalo tsalo added this to the workflow integration: BIDS milestone May 1, 2020
@stale stale bot added the stale label Jul 30, 2020
@tsalo
Copy link
Member Author

tsalo commented Aug 6, 2020

The good news is that, with our new dynamic reports, the order of the components doesn't matter nearly as much! The Kappa rank plot is the only one that retains the same ordering we apply to the components themselves.

@stale stale bot removed the stale label Aug 6, 2020
@tsalo
Copy link
Member Author

tsalo commented Sep 8, 2020

Does anyone have a strong opinion one way or another on this?

Since we wouldn't change the mixing matrix at all, we would probably want to log whether the component time series was flipped in the component table. I don't foresee it being a major problem for denoising, since any denoising step should automatically flip the component's sign to best match the data, right?

@stale stale bot added the stale label Dec 8, 2020
@jbteves
Copy link
Collaborator

jbteves commented Dec 8, 2020

@tsalo I think if there were strong opinions they would have been voiced by now.

@stale stale bot removed the stale label Dec 8, 2020
@tsalo
Copy link
Member Author

tsalo commented Dec 8, 2020

@jbteves good point. I will move forward with this when I have the bandwidth (or if someone else wants to take it on that's great too). Considering the work a number of us put in at BrainHack Donostia on a pure Python version of AROMA, this is more relevant than ever.

@tsalo tsalo added TE-dependence issues related to TE dependence metrics and component selection and removed discussion issues that still need to be discussed labels Dec 8, 2020
@jbteves
Copy link
Collaborator

jbteves commented Dec 8, 2020

Hm, would it make sense to open a PR to your PR for #592 ?

@CesarCaballeroGaudes
Copy link
Contributor

CesarCaballeroGaudes commented Dec 8, 2020

I would also like that the format and order of the ICs is compatible across both packages.

@tsalo
Copy link
Member Author

tsalo commented Dec 8, 2020

I think it happens in metric calculation, right? So probably #591, unless you are still working on an object-oriented version that will go in its own PR.

@jbteves
Copy link
Collaborator

jbteves commented Dec 8, 2020

Whoops, yes, I meant #591.

@ME-ICA ME-ICA deleted a comment from stale bot Jul 6, 2021
@ME-ICA ME-ICA deleted a comment from stale bot Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring issues proposing/requesting changes to the code which do not impact behavior TE-dependence issues related to TE dependence metrics and component selection
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants