Automatic mask creation for MRS shimming based on voxel position #509
Conversation
Thanks for the review and comments! I applied the suggestions. |
There was a problem hiding this comment.
- Add tests for your new function
- I'm not sure if you are using an editor, but most of the comments I am making are simply underlined in my editor to respect PEP conventions. If you are not using an editor, I suggests you do, if not, I suggest familiarizing yourself wit PEP conventions.
- You can add my suggestions directly in GitHub rather than copying what I propose.
Correcting typos in the code Co-authored-by: Alex Dastous <47249340+po09i@users.noreply.github.com>
|
Once a test has been implemented to automatically figure out the center location of MRS data, we can merge this PR |
|
Tests are failing, I'll review when all tests pass :) |
| @click.option('-i', '--input', 'fname_input', type=click.Path(), required=True, | ||
| help="Input path of the fieldmap to be shimmed.") | ||
| @click.option('-r', '--raw_data', type=click.Path(), | ||
| help="Input path of the of the siemens raw-data (supported extention .rda)") |
There was a problem hiding this comment.
| help="Input path of the of the siemens raw-data (supported extention .rda)") | |
| help="Input path of the siemens raw-data (supported extention .rda)") |
| show_default=True, help="Name of the output mask. Supported extensions are .nii or .nii.gz. (default: " | ||
| "(os.curdir, 'mask_mrs.nii.gz'))") |
There was a problem hiding this comment.
No need to tell what the default is in the description. You already see it with the option "show_default"
| show_default=True, help="Name of the output mask. Supported extensions are .nii or .nii.gz. (default: " | |
| "(os.curdir, 'mask_mrs.nii.gz'))") | |
| show_default=True, help="Name of the output mask. Supported extensions are .nii or .nii.gz.") |
| run_subprocess(['spec2nii', 'rda', mrs_raw_data, '-o', tmp]) | ||
| name_nii, ext = os.path.splitext(mrs_raw_data) | ||
| nii_path = os.path.join(tmp, 'sub-1_acq-press-siemens-shim_nuc-H_echo-135_svs.nii.gz') | ||
| nii = nib.load(nii_path) | ||
| header_raw_data = nii.header | ||
| position_sag = header_raw_data['qoffset_x'] | ||
| position_cor = header_raw_data['qoffset_y'] | ||
| position_tra = header_raw_data['qoffset_z'] | ||
|
|
||
| mrs_center = np.array([position_sag, position_cor, position_tra]) | ||
| mrs_voxel_size = header_raw_data['pixdim'][1:4] | ||
| mrs_voxel_size_str = ' '.join(map(str, mrs_voxel_size)) | ||
| mrs_center_str = ' '.join(map(str, mrs_center)) |
There was a problem hiding this comment.
Do not reimplement mask_mrs in the test. Just use what mrs_center_str and what mrs_voxel_size_str contain. The idea is giving a know input and verify that the output is the same across versions of the code. If you include all or some of the implementation of the API, we might not catch these differences.
There was a problem hiding this comment.
Add a test where you give the .rda file as an input
Checklist
Description
A new functionality is added to masking for creating a mask that corresponds to the MRS voxel. The mask is created by reading the MRS voxel position from Siemens' MRS raw data. This mask can be used subsequently to shim the voxel position. The functionality is available through st_mask cli.Linked issues