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

Various improvements on the QC report and resampling module #2157

Merged
merged 55 commits into from
Feb 26, 2019

Conversation

jcohenadad
Copy link
Member

@jcohenadad jcohenadad commented Feb 12, 2019

Various improvements, including:

Another (unrelated) fix:

  • If C2C3 automatic detection fails and is far from the cord, the straightened label will disappear from the FOV, yielding the issue described in ValueError: Array has no mass #2165. In this PR we now raise this error in case the label file is empty, suggesting the user to run the detection manually.

@jcohenadad jcohenadad added enhancement category: improves performance/results of an existing feature sct_qc context: do not merge labels Feb 12, 2019
@jcohenadad jcohenadad changed the title Various improvements on the QC report Various improvements on the QC report and resampling module Feb 12, 2019
@jcohenadad
Copy link
Member Author

jcohenadad commented Feb 15, 2019

@zougloub I would like to pass the new stretch_contrast_method as input param from the SCT function calling generate_qc() (e.g., here). The problem is: I don't want to add a parameter to generate_qc() and then to add_entry. I'm sure there is a more elegant way of doing this (i.e. passing the parameter once instead of three times). Any tips?

@zougloub
Copy link
Contributor

@jcohenadad we can use keyword arguments (**kw) to have less code written, but it's more painful for the users as the API is less explicit (eg. IDE auto-completion doesn't work, documentation is less easy to access).

def a(**kw):
  """
  :param kw: see b()
  """
  ...
  b(**kw)
  ...

def b(**kw):
  """
  :param kw: see c()
  """
  ...
  c(**kw)
  ...

def c(p0=1, p1=2, p2=3):
  """
  :param p0: ...
  """

Not in parent function such as sct_propseg.
Unfinished
Cleaner for images and segmentation.
To distinguish results from co-registration vs. segmentation
@jcohenadad
Copy link
Member Author

jcohenadad commented Feb 24, 2019

I'm having issues with matplotlib's automatic adjustment of figure size upon saving. Namely, the code below (in reports/qc/_save()) does not crop identically across input examples, depending on the size:

        plt.savefig(img_path,
                    format=format,
                    bbox_inches=bbox_inches,
                    pad_inches=pad_inches,
                    transparent=True,
                    dpi=dpi)

Resulting in issues like this where the image and the segmentation is not cropped the same way (highly problematic...):
screen shot 2019-02-24 at 11 34 11

@zougloub do you have any idea how to make sure the saved figure will have a fixed number of pixels? I know matplotlib works with dpi (not fixed pix size), but i'm wondering if there is a way to ensure that, by working on the plt object directly.

@jcohenadad
Copy link
Member Author

@zougloub @perone Regarding the "WARNING" (#2157 (comment)) about manually deleting the resample/ folder after a git pull (because git keeps track of files, not folders), I'm afraid this will cause a lot of trouble in our users. Can you recommend something to deal with this problem? E.g. added some code in the installation to delete empty folders, etc.

@zougloub
Copy link
Contributor

I have no clean solution, just workarounds (eg. renaming the new module resampling or leaving stuff in the __init__.py...), and IMO doing something in the installer is worse.

@charleygros
Copy link
Member

* Moved `spinalcordtoolbox/resample/nipy_resample.py` --> `spinalcordtoolbox/resample.py` for simplicity (folder was not needed)

--> Tested with sct_resample and sct_deepseg_sc, seems all good to me.

* QC folder name now includes milliseconds to avoid overwritting (happens when running with parallel processing)

--> Tested, seems good to me.

* Now retrieve `dataset` and `subject`, according to BIDS convention, and display both in the QC report. Fixes #2091

--> Tested on a dataset with BIDS convention, works!!

* QC report now compatible with uncompressed NIFTI segmentation files. Fixes #2148.

--> Tested, works well.

* Added APRL in first image

--> Tested, really Nice.

@jcohenadad jcohenadad merged commit 94a7010 into master Feb 26, 2019
@jcohenadad jcohenadad deleted the jca/qc-improvements branch February 26, 2019 16:47
@jcohenadad jcohenadad added this to the v4.0.0 milestone Apr 5, 2019
jcohenadad added a commit that referenced this pull request Dec 18, 2019
* nipy_resample.py: Refactored to make it a pure nipy-based function

Also did some cleaning.
Unfinished

* test_resample: New unit test for resample

* slice: Started to refactor (minor changes)

* nipy_resample: Generalized to 4d and fixed compatibility

* resample: Renamed resample_nipy -> resample and move to spinalcord

* sct_deepseg_sc,deepseg_lesion,deepseg_sc: Updated with new resample

* spinalcordtoolbox/resample: Added comments (also in testing)

* reports/slice: Resampling is now done within the QC report

Not in parent function such as sct_propseg.
Unfinished

* sct_propseg: Cleaning

* reports/slice: Cleaning

* reports/slice: Removed legacy msct_image

* sct_deepseg_sc: Implemented new QC changes

* reports/slice: Fixed issue with n>2 dim of self._image

* reports/slice: Now resampling QC image to 0.6mm in-plane

* sct_register_multimodal: Implemented QC (unfinished)

* sct_register_multimodal: Removed requirement for both dseg and iseg

* reports/slice: Changed interpolation nn-->linear

Cleaner for images and segmentation.

* reports/slice: Now binarizing segmentation after resampling

* sct_register_multimodal: Now requiring dest seg for QC

* sct_register_multimodal: Fixed wrong path for QC

* reports/qc: Introduced green and red for segmentation overlay

To distinguish results from co-registration vs. segmentation

* sct_register_multimodal.py: Cleanup

* sct_deepseg_gm: Updated resampling method

* centerline/core: Added TODO

* sct_register_multimodal: Rolled back with QC showing images (not seg)

* reports/qc: Implemented contrast stretching equalization method

Less aggressive

* reports/qc: Added stretch_contrast_method as input param to QcImage

* reports/qc: Now retrive name of dataset, assuming BIDS convention

* reports/qc: Added microseconds in QC folder name

* reports/qc: Now display dataset in QC report

* reports/slice: Prevent resample in every scenario

To accommodate sct_detect_pmj

* reports/qc: Fixed regression with label vertebrae QC

* reports/qc: Now passing stretch_contrast_method as arg

* sct_deepseg_gm: Now setting equalized as contrast method

* sct_deepseg_gm: Fixed-- Now setting equalized as contrast method

* report/qc: Fixed wrong normalization for 2nd image in no_seg_seg()

* resample: Now possible to resample using destination image as ref

* sct_resample_data: Added TODO

* deepseg_gm: Fixed recent change in resample_nipy API

* resample: Cleanup

* sct_register_to_template: Now using equalized mode (in case of z-bias)

* reports/slice: Fixed wrong resampling if im_ref given

* reports/qc: Increased kernel size for equalized contrast stretch

* deepseg,propseg,register_multimodal: Switched back to equalized

With bigger kernel

* reports/slice: Cast np.memmap into np.array

Fixes #2148

* reports/qc: Moved generate_qc() inside reports/qc

* reports/qc: Generalized generate_qc for all covered functions

Added special cases for label_vertebrae and detect_pmj

* reports/qc: Now display orientation labels A-P-R-L in mosaic QC

* reports/qc: Now displaying orientation labels for all layers

* reports/qc: Now using matplotlib OOP API instead of pyplot

Fixes #2138

* reports/qc: Removed matplotlib.pyplot dependency

* vertebrae/detect_c2c3: Fixed wrong path to data

* resample.py -> resampling.py: To prevent conflicts with old resample/

* msct_register_landmarks: Now creating images only with verbose=2

* batch_processing.sh: Fixup, added QC report


Former-commit-id: 94a7010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement category: improves performance/results of an existing feature sct_qc context:
Projects
None yet
3 participants