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

Drop MELODIC step #27

Closed
oesteban opened this issue Nov 11, 2020 · 10 comments · Fixed by #48
Closed

Drop MELODIC step #27

oesteban opened this issue Nov 11, 2020 · 10 comments · Fixed by #48

Comments

@oesteban
Copy link

Eliminate these lines:

https://github.com/Brainhack-Donostia/aroma/blob/3c077ecef03cd44fb2d50d7dbabb37d295afb459/aroma/aroma.py#L219-L220

replacing this with a function that reads in the BOLD data and components and returns the representations necessary in the following steps.

@eurunuela
Copy link
Collaborator

For what I can recall, the idea is to drop those lines and use tedana's ICA function.

@tsalo
Copy link
Member

tsalo commented Nov 11, 2020

Yes, in #22 we have that change, but we can also have BIDS Derivatives-compatible ICA file ingestion and change --md (MELODIC directory) to something more BIDS Derivatives-compatible. We can also drop FEAT directory support.

@oesteban
Copy link
Author

The more responsibilities you take off, the better.

Assume you have all ICA components, mixing parameters, etc available at the input, in the most convenient format. At this point of development, I would not even worry about BIDS naming.

@tsalo
Copy link
Member

tsalo commented Nov 11, 2020

Assume you have all ICA components, mixing parameters, etc available at the input, in the most convenient format. At this point of development, I would not even worry about BIDS naming.

The problem is that that sticks us with MELODIC, since MELODIC is the only ICA method I know of with mixture modeling built in. We could move the ICA + thresholding code I have in #22 into another repository, I guess... but without those changes users won't have a pure Python method of running AROMA.

@oesteban
Copy link
Author

How hard would it be to implement our own mixture modeling (and allow skipping the modeling if MELODIC inputs are provided, to make sure our results are not very off w.r.t. the original implementation and facilitate fast development)?

@tsalo
Copy link
Member

tsalo commented Nov 11, 2020

Depending on how well nipy's Gaussian-Gamma mixture model class works, it should be pretty easy. I haven't tested against the MELODIC outputs yet though. Honestly, without the GGM class, I would have no idea how to implement a mixture model, so I'm really happy I came across it the other day.

@oesteban
Copy link
Author

Oh, I see #1 and #22.

I would give this low priority, first focus on getting a functional prototype working on MELODIC outputs (avoiding their folder structure) and work from there.

@eurunuela
Copy link
Collaborator

I would give this low priority, first focus on getting a functional prototype working on MELODIC outputs (avoiding their folder structure) and work from there.

This makes total sense towards our goal of supporting native-space data. We could ask for the MELODIC output directory and work with that as @oesteban suggests.

@tsalo
Copy link
Member

tsalo commented Nov 12, 2020

This makes total sense towards our goal of supporting native-space data. We could ask for the MELODIC output directory and work with that as @oesteban suggests.

I think @oesteban is proposing that we don't ask for the output directory and instead ask for the individual files. Is that correct?

If we want to be able to take in MELODIC outputs without the folder structure, then we need to have, at minimum:

  1. input file
  2. motion parameters
  3. mixing matrix (new argument)
  4. thresholded z-statistic component maps (new argument)
  5. CSF TPM
  6. brain mask (new argument)

We can generate the power spectra file from the mixing matrix and the TR easily enough. Other than that, does the list seem comprehensive?

@eurunuela
Copy link
Collaborator

Gotcha! Then yes, that list seems reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants