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 : separate deep from shallow WM+CSF in the carpetplot #2744

Merged
merged 9 commits into from
Jan 27, 2023

Conversation

celprov
Copy link
Contributor

@celprov celprov commented Mar 25, 2022

This PR implements the proposition of @effigies (nipreps/niworkflows#651 (comment)) to separate the WM+CSF compartement in the carpetplot into the shallow WM+CSF and the deep WM+CSF corresponding to the aCompCor mask

@pep8speaks
Copy link

pep8speaks commented Mar 25, 2022

Hello @celprov! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-05-27 17:18:00 UTC

fmriprep/workflows/bold/confounds.py Outdated Show resolved Hide resolved
fmriprep/workflows/bold/confounds.py Outdated Show resolved Hide resolved
fmriprep/interfaces/confounds.py Outdated Show resolved Hide resolved
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
celprov and others added 2 commits May 27, 2022 13:17
effigies
effigies previously approved these changes May 27, 2022
@effigies effigies added this to the 22.0.0 milestone May 27, 2022
@effigies effigies dismissed their stale review May 27, 2022 18:39

Reports look problematic

@effigies
Copy link
Member

effigies commented May 27, 2022

Not sure if this is an artifact of our shorter test datasets, but we've lost most of the sections of our carpetplot:

https://output.circle-artifacts.com/output/job/51b32812-2f87-4605-802c-4f98d4973703/artifacts/0/tmp/ds054/derivatives/fmriprep/sub-100185_noerror.html#datatype-figures_desc-summary_run-1_suffix-bold_task-machinegame
fmriprep-limited_carpetplot

Either way, if we figure this out before the full release, I'm happy to include it, but for now I'll release the RC without this.

@celprov
Copy link
Contributor Author

celprov commented May 30, 2022

Hi @effigies, thanks for spotting this issue. I'll try to work on it after I come back from holiday. When is the full release planned for ?

@effigies
Copy link
Member

@celprov targeting early next week, but we can be flexible.

@celprov
Copy link
Contributor Author

celprov commented May 31, 2022

@effigies oh ok very soon. I don't think I will make it then, but I will give it a go if time allows.

@oesteban
Copy link
Member

Not sure if this is an artifact of our shorter test datasets, but we've lost most of the sections of our carpetplot

To be clear, this is not currently happening on master, right? (i.e., it's a problem of this PR)

@effigies
Copy link
Member

Not sure if this is an artifact of our shorter test datasets, but we've lost most of the sections of our carpetplot

To be clear, this is not currently happening on master, right? (i.e., it's a problem of this PR)

Correct.

@effigies effigies modified the milestones: 22.0.0, 23.0.0 Dec 3, 2022
@effigies
Copy link
Member

@celprov When you get a chance, could you merge/rebase this on master?

@celprov
Copy link
Contributor Author

celprov commented Jan 26, 2023

@effigies Good timing! I was precisely thinking about working on this problem this week.

@effigies
Copy link
Member

@celprov Looks like my previous suggestion used bool instead of np.bool_. Latest commit resolves the plotting issue.

Will merge on tests passing.

@effigies
Copy link
Member

image

@effigies effigies merged commit 1df7089 into nipreps:master Jan 27, 2023
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 this pull request may close these issues.

4 participants