Skip to content

include IVIM code from Dr. Amita Shukla-Dave Lab at MSKCC #52

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
May 27, 2025
Merged

include IVIM code from Dr. Amita Shukla-Dave Lab at MSKCC #52

merged 10 commits into from
May 27, 2025

Conversation

locastre
Copy link
Contributor

@locastre locastre commented Mar 4, 2024

We have contributed our codebase for intravoxel incoherent motion (IVIM) code from the MRI-QAMPER MATLAB package, developed by Dr. Amita Shukla-Dave's lab at Memorial Sloan Kettering Cancer Center.

Authors: Eve LoCastro (locastre@mskcc.org), Dr. Ramesh Paudyal (paudyalr@mskcc.org), Dr. Amita Shukla-Dave (davea@mskcc.org)
Institution: Memorial Sloan Kettering Cancer Center
Department: Medical Physics
Address: 321 E 61st St, New York, NY 10022

etpeterson
etpeterson reviewed Mar 5, 2024
View reviewed changes
src/original/ASD_MemorialSloanKettering/MRI-QAMPER_IVIM/utils/nifti_toolbox/license.txt
@@ -0,0 +1,24 @@
Copyright (c) 2014, Jimmy Shen
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be included? It seems to be a separate toolbox. https://www.mathworks.com/matlabcentral/fileexchange/8797-tools-for-nifti-and-analyze-image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey! Thanks for this great submit!
Some questions:
It seems like the toolbox includes some generic toolboxes like the NIFTI toolbox. Do we need that to be part of the package, or could we take it as a requirement?
Also, what does the code do? Is it just fitting, or does it do more processing?

Thanks!

Oliver and Eric

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oliverchampion @etpeterson

The separate nifti_toolbox is relied on extensively to handle nifti input and output in our MATLAB code. This toolbox is freely available on MATLAB FileExchange and as such is subject to the BSD License with the following stipulations:


1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.

2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.

3. Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.

Cited as: Jimmy Shen (2025). Tools for NIfTI and ANALYZE image (https://www.mathworks.com/matlabcentral/fileexchange/8797-tools-for-nifti-and-analyze-image), MATLAB Central File Exchange. Retrieved April 24, 2025.

@locastre
Copy link
Contributor Author

locastre commented Mar 6, 2024 via email

@locastre
Copy link
Contributor Author

locastre commented Mar 6, 2024 via email

@etpeterson
Copy link
Contributor

Just checking in on this because it's been a while. Were you planning on cleaning it or were you expecting someone to take this over?

@oliverchampion
Copy link
Collaborator

Hey! Way too late, but I am reviewing the code. You code is way more extensive then all the other commits, so this may take some more effort/time. But we will get there!
For now there are some things to address.

@oliverchampion
Copy link
Collaborator

You have a demo as "demo_QAMPER_IVIM.m", which is very useful. But as Git gets clogged up fast with files, we do not want nii files in Git. I see you have removed the test data (good!) but obviously the demo now fails.

Luckily, our Git comes with some test data that is stored on Zenodo: https://zenodo.org/records/10696605. This data is automatically downloaded whenever needed, as we coded the git such that it runs utilities/data_simulation/Download_data.py and stores it in "download\Data". I am now running your code with the abdomen.nii.gz data, and this works! However, it required me to (1) download the data manually and (2) make a mask.

So my questions are:
1: can we make sure the data is downloaded automatically whenever we need it for you demo (maybe this is not possible... Then you can refer to the location in the file)
2: is it required to add a mask.nii.gz or can it run without a mask/an automated mask?

@oliverchampion
Copy link
Collaborator

I think there are several files in here that are not needed, such as the .asv files. Could you remove the asv files, and add .asv to the .gitignore?

@locastre
Copy link
Contributor Author

locastre commented Sep 9, 2024

Apologies for the late responses on my part, I have not been receiving the notifications for this thread. I'll work on addressing the ASV and demo scripts now, sending update when they are complete.

@oliverchampion
Copy link
Collaborator

oliverchampion commented Oct 18, 2024 •
edited
Loading

Apologies for the late responses on my part, I have not been receiving the notifications for this thread. I'll work on addressing the ASV and demo scripts now, sending update when they are complete.

@locastre, Any updates :)?

@locastre
Copy link
Contributor Author

Dear @oliverchampion and @etpeterson, thanks for your feedback and patience. I've tried to address all of your questions with several commits this past week.

  • Remove *.asv files and create a .gitignore file to address that pattern for future commits
  • Remove unneeded files that refer to other processing modalities (ie, DCE, T1 FA, etc)
  • Provide a script to download the OSIPI Zenodo demo data and include it in our demo_QAMPER_IVIM.m script
  • If no ROI is provided by the user, the program will default to calculate IVIM parameters for all voxels in the volume.

Please let us know if there are any other issues we should address before merging!

etpeterson
etpeterson approved these changes Apr 27, 2025
View reviewed changes
Copy link
Contributor

@etpeterson etpeterson left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is good to merge. I'll let Oliver do a check before merging though.

@locastre
Copy link
Contributor Author

locastre commented Apr 28, 2025 via email

@oliverchampion
Copy link
Collaborator

Hey!
I've taken the liberty to change some aspects:

  • You've removed the default git-ignore and created one yourself. This means a lot of ignored files will be merged. Fixed this
  • There were some typo's in the matlab file (fulllfile with a L too much)
  • By re-downloading the data to a new location, the data was downloaded twice. I've now pointed to the default data location in the Git, such that the data is shared.

@oliverchampion oliverchampion merged commit c150706 into OSIPI:main May 27, 2025
15 checks passed
@oliverchampion oliverchampion mentioned this pull request May 27, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverchampion oliverchampion oliverchampion left review comments

@etpeterson etpeterson etpeterson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@locastre @etpeterson @oliverchampion