-
Notifications
You must be signed in to change notification settings - Fork 130
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
rodentQC #969
rodentQC #969
Conversation
Hello @eilidhmacnicol, Thank you for updating! Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, Comment last updated at 2022-03-25 14:56:36 UTC |
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.
Looks awesome - thanks for this!
Hey @eilidhmacnicol, I've just cut 22.0.0rc1 and rebased this branch to current master. This is to say, I'm happy to help here and do whatever you need to get this to the finish line. It feels that the better segmentation, the tests, and the better parameters of the head mask can/should be addressed in separate PRs. The improvements to the reportlets pertain to niworkflows/nirodents. |
Hi @oesteban - I've added the option for Atropos segmentation, and it's looking much better now. I agree that segmentation and headmasking can be improved in later commits when we encounter further test data. Marking as ready to merge now. |
One more for style -- can you run
? |
@oesteban - tests are looking good, but I've just realised an issue: I see the NiWorkflows version has been pinned, but |
We will need to finalize spitting out the brain extraction from niworkflows in order to be able to use the new package :( The alternative is too dirty for an official MRIQC release. |
cd59001
to
1fa3b1c
Compare
Okay - some very good news. The NiWorkflows pin issue was (thankfully) a false alarm. I ran an image built on mriqc 22.0.0rc1 with the necessary changes from here and nipreps/nirodents#55 built on top and it ran with no issues. At this stage, the blockers for this PR's merge are: related to
related to
|
9755346
to
f736a17
Compare
I think the tests failing are unrelated to the changes made here and might be beyond the scope of this PR. WDYT @oesteban? Is it time to merge? |
You are going to break master, but considering the type of error it is fair to say that the problem is beyond the scope of this PR. So go ahead and merge. The reason why this is failing is that we probably need to pin |
Actually, you can't merge, I believe. I'm giving you permissions right now - please merge yourself (I believe one should merge their own PRs whenever that is possible/allowed). |
Thanks for opening this pull request and congratulations for taking it to the finish line! It looks like this is your first time contributing to MRIQC. 😄 |
This PR puts in place the foundation to extend MRIQC to other populations, including rodents.
Still to do:
FinaliseInitial head mask parameters for ratsImproving the reportlets for rodent FOVs (although maybe this requires some more work from the niworkflows end)-> moving to NiWorkflowsAdd a test for rats (WDYT?)-> will open a separate issue for thisFor future, it might be worth establishing a dictionary or a set of JSON files that set template choices for each species available in TemplateFlow.
NB: I tried to push to this as a feature branch, rather than straight to master, but I didn't have the credentials.