openneuropet / PET2BIDS

PET2BIDS helps you convert your PET data into BIDS! raw PET scanner files (e.g. ecat, dicom) and additional side files like .e.g excel sheets -- paper @JOSS https://doi.org/10.21105/joss.06067
https://pet2bids.readthedocs.io
MIT License
25 stars 20 forks source link

update python to go along with matlab changes in 9beee53 #278

Closed bendhouseart closed 4 months ago

bendhouseart commented 6 months ago

Having flashbacks with this fix for issue #272


📚 Documentation preview 📚: https://pet2bids--278.org.readthedocs.build/en/278/

bendhouseart commented 6 months ago

This doesn't work as well for python as it did for matlab, nifti's are labeled by the same commit hash (apologies), but they reflect the "same" changes in each ECAT read, see:

Screenshot 2024-02-26 at 1 29 17 PM Screenshot 2024-02-26 at 1 28 59 PM
noahg-neuroimage commented 6 months ago

Looks good to me. Thanks for investigating this!

bendhouseart commented 6 months ago

@noahg-neuroimage are you sure it looks good? The outputs are different now between Matlab and Python when running ecat2nii...

noahg-neuroimage commented 6 months ago

Oh, it seems I misinterpreted the information shown above, haha.

CPernet commented 6 months ago

got to do with fread not the code does do fread(data,int16) but fread(specificdataframes,int16>=int16) which if I understand forces the read data into 16 bit range as oppose to read a 16 bit data frame but values can be out of range -- in python, i think is something like Convert.ToInt16() vs (Int16)

bendhouseart commented 6 months ago

Okay, so still clear as mud to me, but what I think you're saying is that you believe pixel_data_type in the code below is reading 16 bit data but not recognizing it as signed integer 16? And that the trick you're applying in Matlab is designed to surmount how Matlab reads the data as 16 bits, but doesn't cast it to the right type e.g. an int between the ranges of -32768 and 32767?

dt_val = subheader['DATA_TYPE']
                if dt_val == 5:
                    formatting = '>f4'
                    pixel_data_type = numpy.dtype(formatting)
                elif dt_val == 6:
                    # >H is unsigned short e.g. >u2, reverting to int16 e.g. i2 to align with commit 9beee53
                    pixel_data_type = 'i2'
                else:
                    raise ValueError(
                        f"Unable to determine pixel data type from value: {dt_val} extracted from {subheader}")
                # read it into a one dimensional matrix
                pixel_data_matrix_3d = numpy.frombuffer(pixel_data,
                                                        dtype=pixel_data_type,
                                                        count=image_size[0] * image_size[1] * image_size[2]).reshape(
                    *image_size, order='F')
            else:
                raise Exception(f"Unable to determine frame image size, unsupported image type {subheader_type_number}")

The above results in this:

Screenshot 2024-03-06 at 12 11 47 PM

However, I'm not sure that this is helpful as in the python version the numpy.frombuffer() method automatically casts the data as the given type. E.g. as soon as you switch from i2 to >i2 or H you can observe the datatype of the elements in each array change.

See:

Screenshot 2024-03-06 at 12 18 17 PM Screenshot 2024-03-06 at 12 17 43 PM Screenshot 2024-03-06 at 12 17 08 PM

However, I'm a bit skeptical about the differences between >i2 and i2 now as they are both classified as numpy.in16.

I'm going to try force switching the values in pixel_data_matrix_3d after the initial read to see if it has any effect.

bendhouseart commented 5 months ago

I don't disagree, but I'm not happy with the current results as:

  1. The viewed image quality seems to decrease with each step closer I move this to the Matlab way of doing things.
  2. The pixel values in the written NifTI are still off from their Matlab counter part .

There's something I'm failing to account for in this PR.

bendhouseart commented 4 months ago

Seems like we were most of the way there, but added some small fixes that @effigies pointed out, merged his PR, and compared the differences between the output nifitis for the FBP images that caused all this ruckus in the first place.

Subtracting the matlab and python niftis led to a negligible mean difference:

difference max and min: 12.84375, -12.8359375
mean difference: -4.141060278367642e-05

And nib-diff confirms:

(pypet2bids-py3.11) galassiae@MH02276145MLI ecat_testing % nib-diff /Users/galassiae/Data/EcatForGalassi/all_the_niftis/p9816fvat1_fbp_fixed_matlab_3e38914407b_pet.nii /Users/galassiae/Data/EcatForGalassi/all_the_niftis/p9816fvat1_fbp_fixed_python_3e38914407b.nii.gz
These files are different.
Field/File     1:p9816fvat1_fbp_fixed_matlab_3e38914407b_pet.nii      2:p9816fvat1_fbp_fixed_python_3e38914407b.nii.gz       
extents        16384                                                  0                                                      
regular        b'r'                                                   b''                                                    
pixdim         [1.        2.0033126 2.0033126 2.4250002 0.        0.        0.
 0.       ][1.        2.0033126 2.0033126 2.4250002 1.        1.        1.
 1.       ]
xyzt_units     10                                                     2                                                      
cal_max        420487.9                                               420487.88                                              
glmax          420488                                                 0                                                      
glmin          -374226                                                0                                                      
descrip        b'Open NeuroPET ecat2nii.m conversion'                 b'OpenNeuroPET ecat2nii.py conversion'                 
qform_code     0                                                      1                                                      
DATA(md5)      cd6bf9335b7679d370482fd49472898e                       91ddbb7fab6b6e0c726d24e5bc80aaa0                       
DATA(diff 1:)  -                                                      abs: 12.84375, rel: 0.0069204033800270475 

Visual inspection of the difference is as expected:

Screenshot 2024-05-09 at 6 00 02 PM
bendhouseart commented 4 months ago

Something broke the matlab CI, hoping it's just a file getting unzipped and not being able to be found....

bendhouseart commented 4 months ago

Merging this as the matlab unit tests are working fine when run locally, going to create a new issue to fix that fun bug.

@noahg-neuroimage I believe this has been fixed now

CPernet commented 3 months ago

thx for the help @effigies ! this was a bit of a struggle to figure out what was going on