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

[ENH] Generate BIDS Derivatives-compatible outputs #46

Merged
merged 36 commits into from
Nov 22, 2021

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Feb 13, 2021

Closes #5.

Changes proposed in this pull request:

TODO:

  • Update tests
  • Column names
  • Ingestion of BIDS-Derivatives MELODIC outputs
  • BIDS-Derivatives-compatible MELODIC interface?

@tsalo
Copy link
Member Author

tsalo commented Sep 15, 2021

I honestly don't remember what I meant by "Column names", and I think that the MELODIC outputs will have to wait until a different PR.

@tsalo tsalo marked this pull request as ready for review September 15, 2021 15:49
@tsalo
Copy link
Member Author

tsalo commented Sep 15, 2021

I cannot figure out why the classifications are not the same!

@tsalo
Copy link
Member Author

tsalo commented Sep 16, 2021

Hopefully this will be easier to debug once #48 is merged.

@tsalo tsalo added the Help Wanted Extra attention is needed label Sep 22, 2021
@tsalo
Copy link
Member Author

tsalo commented Sep 30, 2021

I'm noticing that our artifact path (for the tests, not the docs build) and output directory for the integration test don't match up:

Artifact path for integration tests: /tmp/data
Output dir: /tmp/pytest-of-neuro/pytest-0/development_fmri/development_fmri/out

Probably worth tackling in a new issue.

EDIT: Opened #50.

@tsalo
Copy link
Member Author

tsalo commented Sep 30, 2021

I'm thinking we should re-generate our test data from main so that we have a cleaner comparison here. @eurunuela WDYT?

@eurunuela
Copy link
Collaborator

I'm noticing that our artifact path (for the tests, not the docs build) and output directory for the integration test don't match up:

Artifact path for integration tests: /tmp/data

Output dir: /tmp/pytest-of-neuro/pytest-0/development_fmri/development_fmri/out

Probably worth tackling in a new issue.

Is it possible this comes from using the nipype docker image?

I'd try rerunning with the miniconda image first.

@eurunuela eurunuela mentioned this pull request Nov 19, 2021
@eurunuela
Copy link
Collaborator

eurunuela commented Nov 22, 2021

Wanna merge his @tsalo ?

For the record, the problem was we were not using a seed and feature_time_series randomizes the selection of the 90% of the rows. I regenerated the benchmark data with a seed in main, added the same seed to the integration tests here, and the tests pass now.

Edit: It would be very useful to merge this for #45 because for some reason I cannot replicate the results in this PR.

@tsalo
Copy link
Member Author

tsalo commented Nov 22, 2021

Sure! Merging now.

@tsalo tsalo merged commit 82f281c into ME-ICA:main Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted Extra attention is needed
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Produce BIDS Derivatives-compatible outputs
2 participants