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

Second pass at image refactoring #1948

Merged
merged 47 commits into from
Aug 10, 2018
Merged

Second pass at image refactoring #1948

merged 47 commits into from
Aug 10, 2018

Conversation

zougloub
Copy link
Contributor

@zougloub zougloub commented Jul 19, 2018

A set of change which:

Done:

  • Trim msct_image and sct_image
  • make use of properties to reduce amount of members (eg. .dim is computed)
  • Add generic free_functions to msct_image:
    • change_{type,orientation}(), which can operate in-place or make copies
    • to_dtype(), which convets type names or dtype to np.dtype
    • change_orientation(), does what it's supposed to do
    • zeros_like() which is often used for creating label images or other images with same shape/header but zero data
    • spatial_crop() which crops images and replaces some sct_crop_image internal calls
  • Improve up msct_image's Image API
  • .change_{type,orientation}() can generate new file names but by default they just void the original one (to prevent overwriting of original data)
  • .save() can modify or not the image contents
  • .save() can save with the current basename to a new directory (typically used in the scenario of copying to tmp.dir, then changing orientation, then saving)
  • members that modify an image return self so calls can be chained
  • add msct_image unit tests
  • Documentation is added, aiming to make the concepts limpid.
  • Functions that write temporary data are the ones responsible for setting up a temporary directory, not their callers

@zougloub
Copy link
Contributor Author

@jcohenadad 's points on #1943 are valid for this:

to be confident with the merge, i suggest we run sct_pipeline (and make sure it has integrity tests when appropriate, and if not, maybe consider adding tests before merging) on sct_testing/large database, on most of the high-level functions that could be affected by the refactoring. Namely (please verify the list below):
Functions that are explicitly modified in this PR:

  • sct_analyze_lesion
  • sct_analyze_texture
  • sct_compute_hausdorff_distance
  • sct_compute_snr
  • sct_create_mask
  • sct_detect_pmj
  • sct_image -getorient
  • sct_image -setorient
  • sct_process_segmentation -p extract_centerline

Functions that do re-orientation and that could be affected:

  • sct_propseg
  • sct_deepseg_sc
  • sct_extract_metric
  • sct_register_to_template

Now I realize there is a lot to test, and most of these functions don't have proper ground truth on sct_testing. So we need to find a good solution.

@@ -446,17 +440,13 @@ def register_data(im_src, im_dest, param_reg, path_copy_warp=None, rm_tmp=True):
os.chdir(tmp_dir)
# save image and seg
fname_src = 'src.nii.gz'
im_src.setFileName(fname_src)
im_src.save()
im_src.save(fname_src)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i really like this feature. i've been dreaming about it for months

@@ -23,10 +23,10 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe don't spend too much time cleaning this function because i've slaughtered it quite a lot already: https://github.com/neuropoly/spinalcordtoolbox/blob/jca/1920-process_seg-csa-perslice/scripts/msct_shape.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no worries, I'll arrange that after your branch is merged

@@ -639,8 +639,7 @@ def save_centerline(self, image=None, fname_output='centerline.sct'):
coord_pix = image.transfo_phys2pix([current_coord])[0]
image_output.data[int(coord_pix[0]), int(coord_pix[1]), int(coord_pix[2])] = float(self.labels_regions[current_label]) + current_dist_rel

image_output.setFileName(fname_output)
image_output.save(type='float32')
image_output.save(fname_output, dtype='float32')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also a very cool feature

return im_out


def orientation(im, ori=None, set=False, get=False, set_data=False, verbose=1, fname_out=None):
def change_orientation_nd(src, orientation, dst=None, inverse=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would suggest to use our conventions for input name, to explicit if this is a file name, an Image object or a nibabel object. So in that case i would replace src with im_src for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wilco

im_out = None
nx, ny, nz, nt, px, py, pz, pt = src.dim

if dst is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment, suggestion: im_dst

sct.printv('sct_convert -i ' + fname_in + ' -o ' + fname_out, verbose, 'code')
im = msct_image.Image(fname_in)
if squeeze_data:
im.data = np.squeeze(im.data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm still puzzled as to why in this function squeeze is on by default. We probably had some issues in the past but i don't remember what. Maybe looking at old closed issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't remember because no reference to issues or useful comment was left in the code... anyway, we'll see what happens when running tests.

if method_type == 'centerline':
convert(method_val, os.path.join(path_tmp, "centerline.nii.gz"))
Image(method_val).change_orientation("RPI").save(os.path.join(path_tmp, "centerline_RPI.nii"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to choose nii over nii.gz? i usually prefer nii.gz for sparse data (centerline, seg) because it compresses amazingly well and for extremely large matrix size it speeds up i/o quite a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for temporary objects.

Here's the rationale:

Today a typical computer

  • performs gzip compression at 100 MB/s
  • writes to mass storage at > 300 MB/s (ssd) or > 150 MB/s (hdd)
  • reads from mass storage at > 500 MB/s (ssd) or > 150 MB/s (hdd)

So using .nii makes the most sense from a speed perspective.

Also, arrays from nii are memory-mapped (whereas those from nii.gz are obviously not).

>>> img1 = nibabel.load("pouet.nii.gz")
>>> type(img1.get_data())
<class 'numpy.ndarray'>
>>> img0 = nibabel.load("pouet.nii")
>>> type(img0.get_data())
<class 'numpy.core.memmap.memmap'>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, i'm convinced. Only for temporary objects though. Storage is still $$$

if method_type == 'point':
convert(method_val, os.path.join(path_tmp, "point.nii.gz"))
Image(method_val).change_orientation("RPI").save(os.path.join(path_tmp, "point_RPI.nii"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about nii

@@ -174,7 +165,7 @@ def create_mask(param):

if method_type == 'centerline':
# get name of centerline from user argument
fname_centerline = 'centerline_RPI.nii.gz'
fname_centerline = 'centerline_RPI.nii'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about nii

@@ -217,11 +211,10 @@ def extract_sagital_slice(self):
def orient2pir(self):
"""Orient input data to PIR orientation."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this "PIR" suddenly comes in, but it has nothing to do with this PR anyways...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because each person has their own frame of reference ;)

dst.data[:] = 0
return dst

def empty_like(img, dtype=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure i understand the origin of the name empty_like. I find it might get confused with zeros_like. It just does a copy of an Image object, right? so why not calling it copy()? Or maybe i am missing something

Copy link
Contributor Author

@zougloub zougloub Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of empty_like() is indeed performing a copy for now, but empty_like is a standard function of numpy which just allocates but doesn't initialize storage. Using this over zeros_like() or copy() makes the intent clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see. so, what would be example cases where one would prefer to use empty_like() vs. copy()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used it for things like:

scripts/sct_compute_hausdorff_distance.py:            self.thinned_image = msct_image.empty_like(self.image)
scripts/sct_compute_hausdorff_distance.py-            self.thinned_image.data = self.zhang_suen(self.image.data)
scripts/sct_compute_hausdorff_distance.py-            self.thinned_image.absolutepath = sct.add_suffix(self.image.absolutepath, "_thinned")

@@ -22,8 +22,8 @@
import pandas as pd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm also doing quite a lot of modifications on this function currently, so many of these modifications might become obsolete.

jcohenadad
jcohenadad previously approved these changes Jul 20, 2018
Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fantastic work! i added a few single comments. mostly minor.

@zougloub
Copy link
Contributor Author

zougloub commented Jul 24, 2018

As I'm reviewing a lot of code and I'm currently facing issues:

  1. sct_apply_transfo can't seem work in unit tests (no displacement observed) in images with certain orientations, which is strange:
    Orientations OK: RAI RAS RPI RPS LAI LAS LPI LPS RIA RIP LIA LIP PLI PLS PIR PIL ILP SLP IAL IPL
    Orientations NG: RSA RSP LSA LSP ARI ARS ALI ALS PRI PRS AIR AIL ASR ASL PSR PSL IRA IRP ILA SRA SRP SLA IAR IPR SAR SAL SPR SPL
  2. Some integration tests not OK for now, I'm working on it: sct_register_to_template sct_segment_graymatter sct_straighten_spinalcord sct_label_vertebrae

@zougloub
Copy link
Contributor Author

I was comparing testing results of the new change_orientation() vs. the old one based on isct_orientation3d and see that while new code passes the unit tests, replacing the code to make use of the old method makes the tests fail.
@jcohenadad you can see for yourself and run pytest unit_testing/test_image.py -k test_change_orientation with #msct_image.change_orientation = old_change_orientation commented or not.
Because of the unit tests checking various properties of the reoriented image:

  • The physical position of points before/after hasn't changed
  • The new orientation is as desired
    I am convinced that there is something that's not working right with isct_orientation3d; I don't think that it's worth it to troubleshoot what's not working in there.

@jcohenadad
Copy link
Member

in response to #1948 (comment), i agree, let's not debug isct_orientation3d. What we could do instead is run sct_pipeline on our large database, using a function that relies on change_orientation, and make sure that results are the same between this branch and master

@zougloub
Copy link
Contributor Author

zougloub commented Jul 26, 2018

Apart from the weird stuff above, some progress...

status: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 99, 99, 0, 0, 0]
Failures: sct_straighten_spinalcord sct_register_to_template

Failures are due to undiagnosed "different translations" caused by newly introduced orientation and phys-pix code.

@zougloub
Copy link
Contributor Author

zougloub commented Jul 27, 2018

The "different translations" problem was due to a mistake when handling in-place orientation changes (which weren't covered by unit tests). Problem solved.

@zougloub zougloub changed the title (WIP) Second pass at image refactoring Second pass at image refactoring Jul 27, 2018
@jcohenadad
Copy link
Member

jcohenadad commented Aug 6, 2018

tested 38cd048 on sct_testing_data/t2. Below are the center of FOV (in physical coordinate) for each situation. It should be the same for all situations:

t2

-6.719334
2.286655
7.67828

sct_image -i t2.nii.gz -setorient RPI (version: master)

-5.219334
6.286655
10.17828

NOT THE SAME --> FAIL 👎

sct_image -i t2.nii.gz -setorient RPI (version: 38cd048f5f86db70ad12e67ebd5e5156f1ea1bf2)

-6.719334
2.286655
7.67828

THE SAME --> PASS 👍

fslswapdim t2.nii.gz RL PA IS (sanity check: FSL)

-6.719334
2.286655
7.67828

THE SAME --> EXPECTED

At least with this image, this PR seems to fix this issue, which is pretty awesome. I'll do more testing on this PR.

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testing this PR using batch_processing. I got this:

UserWarning: Deprecated, this function has no value over msct_image.change_orientation()
jcohen@joplin:~$ code/sct/batch_processing.sh
Removing /home/jcohen/qc_batch_processing folder.
Started at: 2018-08-06_05:54:57 PM

--
Spinal Cord Toolbox ((HEAD/38cd048f5f86db70ad12e67ebd5e5156f1ea1bf2)
Running /home/jcohen/code/sct/scripts/sct_download_data.py -d sct_example_data

Trying URL: https://osf.io/kjcgs/?action=download
Downloading 20180525_sct_example_data.zip...
Status: 100%|#######################################################################################################################################| 44.3M/44.3M [00:00<00:00, 48.1MB/s]

Check if folder already exists...
WARNING: Folder sct_example_data already exists. Removing it...
rm -rf sct_example_data

Unzip data to: /home/jcohen

Remove temporary file...
Done!

Total processing time: 0 min 3 s


--
Spinal Cord Toolbox ((HEAD/38cd048f5f86db70ad12e67ebd5e5156f1ea1bf2)
Running /home/jcohen/code/sct/scripts/sct_propseg.py -i t2.nii.gz -c t2
Loaded /home/jcohen/sct_example_data/t2/t2.nii.gz orientation LPI shape (64, 320, 320)
Loaded /home/jcohen/sct_example_data/t2/t2.nii.gz orientation LPI shape (64, 320, 320)
Loaded /home/jcohen/sct_example_data/t2/t2.nii.gz orientation LPI shape (64, 320, 320)
Detecting the spinal cord using OptiC
cp /home/jcohen/sct_example_data/t2/t2.nii.gz /tmp/sct-20180806175503.328997-8sa4WR
Loaded /home/jcohen/sct_example_data/t2/t2.nii.gz orientation LPI shape (64, 320, 320)
Saving image to t2_int16.nii.gz (/tmp/sct-20180806175503.328997-8sa4WR/t2_int16.nii.gz) orientation LPI shape (64, 320, 320)
Loaded t2_int16_RPI.nii (/tmp/sct-20180806175503.328997-8sa4WR/t2_int16_RPI.nii) orientation RPI shape (64, 320, 320)
cp t2_centerline_optic.nii.gz /home/jcohen/sct_example_data/t2/./
rm -rf /tmp/sct-20180806175503.328997-8sa4WR
isct_propseg -t t2 -o ./ -verbose -i /home/jcohen/sct_example_data/t2/t2.nii.gz -init-centerline ./t2_centerline_optic.nii.gz -centerline-binary # in /home/jcohen/sct_example_data/t2
Initialization - using given centerline
Total propagation length = 208.344 mm

Segmentation finished. To view results, type:
fslview /home/jcohen/sct_example_data/t2/t2.nii.gz ./t2_seg.nii.gz &

Check consistency of segmentation...

Create temporary folder (/tmp/sct-20180806175518.562257-propseg-Y3GC4L)...
Loaded ./t2_seg.nii.gz (/home/jcohen/sct_example_data/t2/t2_seg.nii.gz) orientation LPI shape (64, 320, 320)
Saving image to /tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.segmentation.nii.gz orientation LPI shape (64, 320, 320)
Loaded ./t2_centerline.nii.gz (/home/jcohen/sct_example_data/t2/t2_centerline.nii.gz) orientation LPI shape (64, 320, 320)
Saving image to /tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.centerline.nii.gz orientation LPI shape (64, 320, 320)
Loaded tmp.segmentation.nii.gz (/tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.segmentation.nii.gz) orientation LPI shape (64, 320, 320)
tmp.segmentation.nii.gz
Loaded tmp.segmentation.nii.gz (/tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.segmentation.nii.gz) orientation LPI shape (64, 320, 320)
/home/jcohen/code/sct/scripts/sct_image.py:556: UserWarning: Deprecated, this function has no value over msct_image.change_orientation()
  warnings.warn("Deprecated, this function has no value over msct_image.change_orientation()")
Saving image to tmp.segmentation_RPI.nii.gz (/tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.segmentation_RPI.nii.gz) orientation RPI shape (64, 320, 320)
Saving image to tmp.segmentation_RPI.nii.gz (/tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.segmentation_RPI.nii.gz) orientation RPI shape (64, 320, 320)
tmp.centerline.nii.gz
Loaded tmp.centerline.nii.gz (/tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.centerline.nii.gz) orientation LPI shape (64, 320, 320)
Saving image to tmp.centerline_RPI.nii.gz (/tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.centerline_RPI.nii.gz) orientation RPI shape (64, 320, 320)
Saving image to tmp.centerline_RPI.nii.gz (/tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.centerline_RPI.nii.gz) orientation RPI shape (64, 320, 320)
Loaded tmp.segmentation_RPI.nii.gz (/tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.segmentation_RPI.nii.gz) orientation RPI shape (64, 320, 320)
Loaded tmp.centerline_RPI.nii.gz (/tmp/sct-20180806175518.562257-propseg-Y3GC4L/tmp.centerline_RPI.nii.gz) orientation RPI shape (64, 320, 320)

@zougloub
Copy link
Contributor Author

zougloub commented Aug 7, 2018

I've left the function around for the next refactoring pass.

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i got the following bug when running batch_processing:

--
Spinal Cord Toolbox (cJ/more-image/38cd048f5f86db70ad12e67ebd5e5156f1ea1bf2)
Running /Users/julien/code/sct/scripts/sct_create_mask.py -i mt1.nii.gz -p center -size 45mm

Create temporary folder (/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe)...

Check orientation...
Loaded mt1.nii.gz (/Users/julien/sct_example_data/mt/mt1.nii.gz) orientation LPI shape (192, 192, 22)
.. LPI
Loaded mt1.nii.gz (/Users/julien/sct_example_data/mt/mt1.nii.gz) orientation LPI shape (192, 192, 22)
Saving image to /var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_RPI.nii orientation RPI shape (192, 192, 22)

Get dimensions of data...
Loaded data_RPI.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_RPI.nii) orientation RPI shape (192, 192, 22)
  192 x 192 x 22 x 1

Create line...
cp data_RPI.nii line.nii
sct_maths -i line.nii -mul 0 -o line.nii # in /private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe
sct_label_utils -i line.nii -o line.nii -create-add 96,96,0,1: 96,96,1,1: 96,96,2,1: 96,96,3,1: 96,96,4,1: 96,96,5,1: 96,96,6,1: 96,96,7,1: 96,96,8,1: 96,96,9,1: 96,96,10,1: 96,96,11,1: 96,96,12,1: 96,96,13,1: 96,96,14,1: 96,96,15,1: 96,96,16,1: 96,96,17,1: 96,96,18,1: 96,96,19,1: 96,96,20,1: 96,96,21,1 # in /private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe

Create mask...
Loaded data_mask0.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask0.nii) orientation RPI shape (192, 192)
Loaded data_mask1.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask1.nii) orientation RPI shape (192, 192)
Loaded data_mask2.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask2.nii) orientation RPI shape (192, 192)
Loaded data_mask3.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask3.nii) orientation RPI shape (192, 192)
Loaded data_mask4.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask4.nii) orientation RPI shape (192, 192)
Loaded data_mask5.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask5.nii) orientation RPI shape (192, 192)
Loaded data_mask6.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask6.nii) orientation RPI shape (192, 192)
Loaded data_mask7.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask7.nii) orientation RPI shape (192, 192)
Loaded data_mask8.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask8.nii) orientation RPI shape (192, 192)
Loaded data_mask9.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask9.nii) orientation RPI shape (192, 192)
Loaded data_mask10.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask10.nii) orientation RPI shape (192, 192)
Loaded data_mask11.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask11.nii) orientation RPI shape (192, 192)
Loaded data_mask12.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask12.nii) orientation RPI shape (192, 192)
Loaded data_mask13.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask13.nii) orientation RPI shape (192, 192)
Loaded data_mask14.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask14.nii) orientation RPI shape (192, 192)
Loaded data_mask15.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask15.nii) orientation RPI shape (192, 192)
Loaded data_mask16.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask16.nii) orientation RPI shape (192, 192)
Loaded data_mask17.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask17.nii) orientation RPI shape (192, 192)
Loaded data_mask18.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask18.nii) orientation RPI shape (192, 192)
Loaded data_mask19.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask19.nii) orientation RPI shape (192, 192)
Loaded data_mask20.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask20.nii) orientation RPI shape (192, 192)
Loaded data_mask21.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask21.nii) orientation RPI shape (192, 192)
Loaded data_mask0.nii (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/data_mask0.nii) orientation RPI shape (192, 192)
Saving image to mask_RPI.nii.gz (/private/var/folders/6f/wy6ljmx9453cgth2qwv5l1s80000gn/T/sct-20180807151636.362374-create_mask-W2d6Xe/mask_RPI.nii.gz) orientation RPI shape (192, 192, 22)
Traceback (most recent call last):
  File "/Users/julien/code/sct/scripts/sct_create_mask.py", line 383, in <module>
    main()
  File "/Users/julien/code/sct/scripts/sct_create_mask.py", line 83, in main
    create_mask(param)
  File "/Users/julien/code/sct/scripts/sct_create_mask.py", line 233, in create_mask
    im_out.header = Image(param.fname_data).header
  File "/Users/julien/code/sct/scripts/msct_image.py", line 371, in __init__
    self.loadFromPath(param, verbose)
  File "/Users/julien/code/sct/scripts/msct_image.py", line 456, in loadFromPath
    self.im_file = nibabel.load(path)
  File "/Users/julien/code/sct/python/lib/python2.7/site-packages/nibabel/loadsave.py", line 40, in load
    raise FileNotFoundError("No such file: '%s'" % filename)
nibabel.py3k.FileNotFoundError: No such file: 'mt1.nii.gz'
Sentry is attempting to send 1 pending error messages
Waiting up to 10 seconds
Press Ctrl-C to quit
No handlers could be found for logger "sentry.errors"
Note: you can opt out of Sentry reporting by editing the file ${SCT_DIR}/bin/sct_launcher and delete the line starting with "export SENTRY_DSN"
Total processing time: 0 min 4 s

to reproduce:

cd sct_example_data/mt
sct_create_mask -i mt1.nii.gz -p center -size 45mm

@jcohenadad
Copy link
Member

bug on batch_processing on b8d12e8

--
Spinal Cord Toolbox (cJ/more-image/b8d12e82de8c05127ba29e2cc7a2744c1c104c2b)
Running /home/jcohen/code/sct/scripts/sct_compute_mtsat.py -mt mt1_crop.nii.gz -pd mt0_reg.nii.gz -t1 t1w_reg.nii.gz -trmt 30 -trpd 30 -trt1 15 -famt 9 -fapd 9 -fat1 15
Load data...
Loaded mt1_crop.nii.gz (/home/jcohen/sct_example_data/mt/mt1_crop.nii.gz) orientation LPI shape (61, 57, 22)
Loaded mt0_reg.nii.gz (/home/jcohen/sct_example_data/mt/mt0_reg.nii.gz) orientation LPI shape (61, 57, 22)
Loaded t1w_reg.nii.gz (/home/jcohen/sct_example_data/mt/t1w_reg.nii.gz) orientation LPI shape (61, 57, 22)
Compute T1 map...
Compute A...
Compute MTsat...
Generate output files...
Traceback (most recent call last):
  File "/home/jcohen/code/sct/scripts/sct_compute_mtsat.py", line 98, in <module>
    run_main(arguments)
  File "/home/jcohen/code/sct/scripts/sct_compute_mtsat.py", line 86, in run_main
    fname_b1map=args.b1map, fname_mtsat=args.omtsat, fname_t1map=args.ot1map, verbose=1)
  File "/home/jcohen/code/sct/spinalcordtoolbox/mtsat/mtsat.py", line 133, in compute_mtsat_from_file
    fname_mtsat = os.path.join(nii_mt.path, "mtsat.nii.gz")
AttributeError: 'Image' object has no attribute 'path'

@jcohenadad
Copy link
Member

jcohenadad commented Aug 8, 2018

output of batch_processing on 374e8d0 (cJ/more-image)

t2/CSA:   78.703221, 1.760750
mt/MTR:   2018/08/08 - 15:52:32,/home/jcohen/sct_example_data/mt/mtr.nii.gz,map,"2,3,4,5","15,16,17,11,12,13,14,8,9,10,4,5,6,7",51,white matter,980.883843843,54.1147920284,0
t2s/CSA_GM:   13.240696, 3.370950
t2s/CSA_WM:   65.202414, 3.112070
dmri/FA:  2018/08/08 - 15:57:04,/home/jcohen/sct_example_data/dmri/dti_FA.nii.gz,wa,Unknown,"2,3,4,5,6,7,8,9,10,11,12,13,14",4,WM left lateral corticospinal tract,80.5077761052,0.758846851538,0.167810495669
dmri/FA:  2018/08/08 - 15:57:04,/home/jcohen/sct_example_data/dmri/dti_FA.nii.gz,wa,Unknown,"2,3,4,5,6,7,8,9,10,11,12,13,14",5,WM right lateral corticospinal tract,81.3368825917,0.785060184288,0.143668976472

master

t2/CSA:   78.695073, 1.757627
mt/MTR:   2018/08/08 - 17:22:52,/home/jcohen/sct_example_data/mt/mtr.nii.gz,map,"2,3,4,5","15,16,17,11,12,13,14,8,9,10,4,5,6,7",51,white matter,980.325349487,54.1561304303,0
t2s/CSA_GM:   13.240696, 3.370950
t2s/CSA_WM:   65.202414, 3.112070
dmri/FA:  2018/08/08 - 17:27:26,/home/jcohen/sct_example_data/dmri/dti_FA.nii.gz,wa,Unknown,"2,3,4,5,6,7,8,9,10,11,12,13,14",4,WM left lateral corticospinal tract,80.4305730405,0.753208526827,0.173244755457
dmri/FA:  2018/08/08 - 17:27:26,/home/jcohen/sct_example_data/dmri/dti_FA.nii.gz,wa,Unknown,"2,3,4,5,6,7,8,9,10,11,12,13,14",5,WM right lateral corticospinal tract,80.8171689672,0.787411637177,0.14199512341

v3.2.3:

t2/CSA:   78.182308, 1.762403
mt/MTR:   51, white matter, 1547.984915, 55.426165, 0.000000
t2s/CSA_GM:   13.282941, 3.440752
t2s/CSA_WM:   65.160860, 3.175766
dmri/FA:  4, WM left lateral corticospinal tract, 81.115248, 0.755613, 0.174256
dmri/FA:  5, WM right lateral corticospinal tract, 81.458362, 0.783391, 0.145285

Results are slightly different between master and cJ/more-image. @zougloub any idea what could cause these difference? Just trying to get a sense if these seemingly small differences could become large in other scenario.

Note: the issue with output formatting will be addressed in #1987

@zougloub
Copy link
Contributor Author

zougloub commented Aug 9, 2018

Orientation changes and physical coordinate computation is done differently (correctly?) now and that could explain slightly different results. I don't see how they would be less accurate than before, so if numbers change, I'd trust the new ones. The locus of these changes is the phys2pix, pix2phys and change_orientation functions, whose diff you could review (and I bet that you'll favor the new versions).
I have also observed, in conditions which make the old isct_orientation binary and the new code produce same results, that the results were slightly different, and the numerical accuracy of the new code was better (I haven't kept data to back this statement).

@jcohenadad
Copy link
Member

yeah, i also suspect that results could be more trustable now. I'd still be curious to compare results of test_sct_register_to_template. Currently running it-- comparative results should be out in ~2h.

@jcohenadad jcohenadad added enhancement category: improves performance/results of an existing feature msct_image labels Aug 9, 2018
@jcohenadad jcohenadad added this to the v3.2.4 milestone Aug 9, 2018
@jcohenadad
Copy link
Member

added tags/milestone because since the produced results are slightly different, we need to justify where those changes come from (hence listing this PR in the changelog).

@jcohenadad
Copy link
Member

jcohenadad commented Aug 10, 2018

results of sct_register_to_template -i t2/t2.nii.gz -s t2/t2_seg_manual.nii.gz -l t2/labels.nii.gz -igt /home/jcohen/code/sct/data/PAM50/template/PAM50_cord.nii.gz using sct_testing/large

master-4208eff65079fe7ee3626e0099c5fc54b1c01abe:

Duration: 2242s
Passed: 151/432
Crashed: 225/432
Mean: {'dice_anat2template': 0.9108050585338047, 'dice_template2anat': 0.9215961655033242, 'duration': 488.3033762000225}
STD: {'duration': 522.7553536128281, 'dice_template2anat': 0.029181743744652464, 'dice_anat2template': 0.023049261376646382}

cJ/more-image-9b6cdfc0d2967905411c9f1e05987fa4e8fb7aa9:

Duration: 1808s
Passed: 152/432
Crashed: 225/432
Mean: {'dice_anat2template': 0.910986766710356, 'dice_template2anat': 0.9215147868289546, 'duration': 364.76865527751266}
STD: {'duration': 389.7401216208057, 'dice_template2anat': 0.02895368689489166, 'dice_anat2template': 0.022843669615022065}

conclusion: negligible changes.
Note: the "crash" are caused by the label file (specified by "-l") not existing. The output status should be 200, but it is 1. Issue opened: #1990

@zougloub zougloub merged commit d200d8b into master Aug 10, 2018
@zougloub zougloub deleted the cJ/more-image branch August 10, 2018 02:44
jcohenadad pushed a commit that referenced this pull request Dec 18, 2019
Second pass at image refactoring

Former-commit-id: d200d8b
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figuring out orientation so set_orientation() can be deprecated in favor of an updated change_orientation().
2 participants