nipreps / nirodents

Apache License 2.0
12 stars 3 forks source link

ENH: add mosaic plots and wrapper #16

Closed eilidhmacnicol closed 4 years ago

eilidhmacnicol commented 4 years ago

Adds mosaic plot function and a wrapper which calls the brain extraction workflow and generates mosaic plots of outputs. Also includes some fixes to the brain extraction workflow, importantly adding data grabber and data sink, and some small changes to placate linter.

pull-assistant[bot] commented 4 years ago
Score: 0.96

Best reviewed: commit by commit


Optimal code review plan (1 warning)

     FIX: updated workflow input and outputs      Update .gitignore
ENH: added functionality for T1w images
> `...s/workflows/brainextraction.py` 50% changes removed in FIX: update brainext...
     ENH: add mosaic plot scripts      Apply suggestions from code review      Update brainextraction.py      Move and rename Viz      FIX: CLI interface files updated      FIX: update nipype version dependency      FIX: update brainextraction.py

Powered by Pull Assistant. Last update 2d8bafd ... 04c8313. Read the comment docs.

eilidhmacnicol commented 4 years ago

Thanks for your comments! My plan is to look at them all today.

eilidhmacnicol commented 4 years ago

Okay, @oesteban: I'd appreciate your thoughts on a few things before moving on with this, but I think this is almost ready to merge. (This is mostly summarising comments from up and down the page, so they're in the same place).

  1. I've added the plot CLI but I haven't tested it on Sherlock. I think it might be better to get this pull request sorted and then try again with Sherlock, based on point 2.
  2. I still don't understand why I had to prefix the brain extraction wf with data, but sherlock would not run the scripts any other way. I've left your suggestions open just now; if you think we should go ahead and close this pull request before I try to test it more on Sherlock, then I'll make the changes you suggested (i.e. remove the data). If not, this will take some more time as I test all this on Sherlock, likely after Easter weekend.
  3. I've left the viz.py file in because it was taken from MRIQC which is currently not a dependency. If you think there's something more appropriate that can be imported from niworkflows, I'm happy to take out the file and make the changes in the CLI.
  4. WRT the test for the masking function, I wasn't sure what you meant. Is it a standalone script or do we implement that in the CLI?

I think that's everything else resolved, though.

eilidhmacnicol commented 4 years ago

Okay, two things remaining if you find time to check this today.

The first is the mosaic plot function from MRIQC versus niworkflows. If you think there's an appropriate function in niworkflows, then we can get rid of the viz.py that I have which is a trimmed-down version of the script from MRIQC.

The second is the test function suggestion. I forgot to ask you what you meant on zoom.

oesteban commented 4 years ago

The first is the mosaic plot function from MRIQC versus niworkflows.

I think we want to start with MRIQC in this case. There are good alternatives in NiWorkflows, but they reorient the image and I believe we don't want that in this case.

The second is the test function suggestion.

Integrate some testing suite (I'd go with pytest which is basically the standard these days) and call your plotting function on the T2w of the template with the mask. That should give us a pretty picture of the template.

oesteban commented 4 years ago

I think we can do the test function on another PR, so we feel some progress merging this :)