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

BAMF - NNUnet Prostate MR #47

Merged
merged 17 commits into from
Mar 19, 2024
Merged

Conversation

rahul99
Copy link

@rahul99 rahul99 commented Aug 29, 2023

No description provided.

@rahul99
Copy link
Author

rahul99 commented Aug 29, 2023

@LennyN95 , Hi Leo, the issue about running container on MR scans persists. This PR is also about another MR model! I look forward to your suggestions!

@LennyN95
Copy link
Member

Hi Rahul,

NNUnetRunner takes CT scans by default, as you can see in the source file here: @IO.ConfigInput('in_data', 'nifti:mod=ct', the="input data to run nnunet on")

Please add the following line to your config:

NNUnetRunner:
   # ...
   in_data: nifti:mod=mr

@LennyN95
Copy link
Member

Please also prepare a meta.json file in your model folder, you can find instructions on how to set-up the file here: https://github.com/MHubAI/documentation/blob/main/documentation/mhub_models/model_json.md

I prepared an example for TotalSegmentator here:
https://gist.github.com/LennyN95/e99e43cc15eabf72762a6a47a5530d7b

Best,
Leo.

@LennyN95 LennyN95 changed the title mhub resources for bamf-nnunet-mr-prostate model BAMF - NNUnet Prostate MR Aug 31, 2023
@rahul99
Copy link
Author

rahul99 commented Sep 3, 2023

Hi Leo, I have updated the config and added meta.json. Let me know if I am missing something. Thanks!

@LennyN95
Copy link
Member

LennyN95 commented Sep 3, 2023

Hi Rahul,

The meta.json must be placed in the model's root folder (models/bamf_nnunet_mr_prostate in this case).
Please have a look at the two resources I posted to prepare your meta.json. It's a metadata file we require for all MHub models (it's different from the DCMQI JSON for Dicomseg conversion).

Also, I spotted a small typo in your config that will prevent the NNUnetRunner from receiving any files. I added a comment to the line.

Best,
Leo.

@rahul99 rahul99 marked this pull request as ready for review September 22, 2023 19:03
@rahul99
Copy link
Author

rahul99 commented Sep 22, 2023

Hey Leo, I have made the changes we discussed yesterday for Prostate MR model. I observed something interesting. If I remove the json_config_path from DsegConverter module (as recommended), the execution throws error. If I put it back, it works all right as we saw yesterday. Please advice on how to proceed with this. For your reference, I am attaching screenshot of before and after removing json_config_path to the config
Screenshot 2023-09-22 at 12 01 34 PM
Screenshot 2023-09-22 at 11 58 57 AM

@LennyN95
Copy link
Member

Hi Rahul,

This looks like there is no roi provided for the nnunet runner. Try adding a comma separated list of SegDB ids for each segmentation of the nnunet (without background), then the DsegConverter will be able to generate the dseg config correctly.

Keep me posted if that works ;)

Best,
Leo.

@rahul99
Copy link
Author

rahul99 commented Sep 26, 2023

Hi Leo,

Sorry I do not see an entry for prostate segment id in segDB: https://github.com/MHubAI/SegDB/blob/main/segdb/data/segmentations.csv

Could you please make that entry and then I should be able to refer to it?

Thanks,
Rahul

@LennyN95
Copy link
Member

LennyN95 commented Oct 27, 2023

Hi Rahul,

How is the progress?

I added the prostate structure to SegDB with ID PROSTATE.

You need to make sure you pull the latest base image (mhubai/base:latest) to get the latest version of SegDB.
I'd always use --no-cache flag in the docker build command and suggest double checking the image creation time from docker images.

Best,
Leo.

@fedorov
Copy link
Member

fedorov commented Nov 14, 2023

@rahul99 @vanossj when do you think you would have time to finish this?

We are very interested to use this model in another study that is led by Cosmin @ccosmin97.

Rahul Soni added 8 commits November 14, 2023 15:34
    Supervised to supervised
- In /summary/model/cmpapproach
    "#D, Emsemble" to "3D"
- /details/licence to /details/license
@fedorov
Copy link
Member

fedorov commented Dec 13, 2023

@LennyN95 how is testing going with this model? Maybe @ccosmin97 can help with this?

@LennyN95
Copy link
Member

@fedorov we're waiting for input from the collaborators.

@LennyN95
Copy link
Member

NOTE: The model is ready for testing.

@rahul99 To verify that the model runs correctly, we need you to run each model for each workflow (workflow means config, e.g., the default workflow is the default.yml file) and to send us the results (output exactly as generated by the MHub container). Ideally, you could provide us with the results in a structure like so: <model_name>/<workflow_name>/<output>. Please do not alter the output, we will run the model on our side and compare the outputs to verify that the model works as expected.

@fedorov
Copy link
Member

fedorov commented Dec 20, 2023

@rahul99 for testing the model, please provide the following for sample MR series that are known to produce acceptable results with this model:

*SeriesInstanceUID for all input series

  • aws_url for the bucket folders containing the series files
  • version of IDC data that you used for getting the above

If you pick one of the cases that are already accompanied by a segmentation produced by this model in IDC (they have just been added yesterday in IDC v17), you can also include the items above for the SEG series.

@fedorov
Copy link
Member

fedorov commented Dec 27, 2023

please provide the following for sample MR series that are known to produce acceptable results with this model:

Upon discussing this with @ccosmin97, I realized it is probably unclear how to do what I asked in the above.

If you use IDC Portal, you can navigate to the row with each of the series you use in your testing, and you will see the button to copy SeriesInstanceUID to the clipboard as shown in the screenshot below.

image

If you click the download button for the series export, you will see the command line to download the content of the series folder. By default, that download is done from the AWS bucket, and you can get the AWS folder from that path - see highlighted below.

image

Finally, you can get the version of IDC data from the front page of the portal:

image

Once you collect all of this information, you can download the source images from the bucket URL, confirm that everything works as expected, and share the details with the MHub.AI developers for integrating the model.

@LennyN95 hope this makes sense, happy to discuss how to refine/simplify this.

@LennyN95 LennyN95 added the REQUEST TEST Attach this label to your PR when your submission is ready to be tested by us. label Jan 22, 2024
@LennyN95
Copy link
Member

LennyN95 commented Jan 22, 2024

/test

sample:
  idcv: 17.0
  data:
    - SeriesInstanceUID: 1.3.6.1.4.1.14519.5.2.1.7311.5101.160028252338004527274326500702
      aws: 76d402d3-ea3f-464b-a5f2-f4f7ff813eab
      path: dicom

reference:
  url: https://www.dropbox.com/scl/fi/2o12d46elqvinbfbf3xr6/bamf_nnunet_mr_prostate.zip?rlkey=tlcy6mk2cdo1xvjx4fnxful1e&dl=0

Test Results (2024-01-23 13:49:27)
findings:
- checker: SizeCheck
  facts:
  - description: file size is larger than reference
    label: Size Difference
    subpath: ''
    value: 2
  meta: {}
  path: /app/test/src/1.3.6.1.4.1.14519.5.2.1.7311.5101.160028252338004527274326500702/bamf_nnunet_mr_prostate.seg.dcm
- checker: DicomsegContentCheck
  facts:
  - description: "Exception thrown in SimpleITK LabelOverlapMeasuresImageFilter_Execute:\
      \ /tmp/SimpleITK-build/ITK-prefix/include/ITK-5.3/itkImageSink.hxx:239:\nITK\
      \ ERROR: LabelOverlapMeasuresImageFilter(0x28dcaa0): Inputs do not occupy the\
      \ same physical space! \nInputImage Origin: [-5.7636417e+01, 1.1161974e+02,\
      \ -5.0827725e+01], InputImageTargetImage Origin: [-5.7634422e+01, 1.1161975e+02,\
      \ -5.0827728e+01]\n\tTolerance: 5.0000000e-07\nInputImage Direction: 9.9999921e-01\
      \ -2.0706479e-10 -1.2568636e-03\n-3.9861051e-04 -9.4837644e-01 -3.1714660e-01\n\
      -1.1919797e-03 3.1714685e-01 -9.4837569e-01\n, InputImageTargetImage Direction:\
      \ 9.9999914e-01 8.3240541e-06 -1.3080033e-03\n-4.0693478e-04 -9.4837644e-01\
      \ -3.1714659e-01\n-1.2431195e-03 3.1714685e-01 -9.4837563e-01\n\n\tTolerance:\
      \ 1.0000000e-06\n"
    label: Comparison Fail
    subpath: 'segment #1'
    value: -1
  meta: {}
  path: /app/test/src/1.3.6.1.4.1.14519.5.2.1.7311.5101.160028252338004527274326500702/bamf_nnunet_mr_prostate.seg.dcm

Test Results (24.01.29_13.36.48_9caE8eLF4g)
checked_files:
- path: /app/test/src/1.3.6.1.4.1.14519.5.2.1.7311.5101.160028252338004527274326500702/bamf_nnunet_mr_prostate.seg.dcm
  checker: DicomsegContentCheck
  findings:
  - label: Comparison Fail
    description: "Exception thrown in SimpleITK LabelOverlapMeasuresImageFilter_Execute:\
      \ /tmp/SimpleITK-build/ITK-prefix/include/ITK-5.3/itkImageSink.hxx:239:\nITK\
      \ ERROR: LabelOverlapMeasuresImageFilter(0x2d3b840): Inputs do not occupy the\
      \ same physical space! \nInputImage Origin: [-5.7636417e+01, 1.1161974e+02,\
      \ -5.0827725e+01], InputImageTargetImage Origin: [-5.7634422e+01, 1.1161975e+02,\
      \ -5.0827728e+01]\n\tTolerance: 5.0000000e-07\nInputImage Direction: 9.9999921e-01\
      \ -2.0706479e-10 -1.2568636e-03\n-3.9861051e-04 -9.4837644e-01 -3.1714660e-01\n\
      -1.1919797e-03 3.1714685e-01 -9.4837569e-01\n, InputImageTargetImage Direction:\
      \ 9.9999914e-01 8.3240541e-06 -1.3080033e-03\n-4.0693478e-04 -9.4837644e-01\
      \ -3.1714659e-01\n-1.2431195e-03 3.1714685e-01 -9.4837563e-01\n\n\tTolerance:\
      \ 1.0000000e-06\n"
    subpath: 'segment #1'
    info: -1
summary:
  files_missing: 0
  files_extra: 0
  checks:
    DicomsegContentCheck:
      files: 1
      findings:
        Comparison Fail: 1
conclusion: false

@ccosmin97
Copy link
Contributor

ccosmin97 commented Feb 6, 2024

/test

sample:
  idcv: 17.0
  data:
    - SeriesInstanceUID: 1.3.6.1.4.1.14519.5.2.1.7311.5101.160028252338004527274326500702
      aws: 76d402d3-ea3f-464b-a5f2-f4f7ff813eab
      path: dicom

reference:
  url: https://www.dropbox.com/scl/fi/2o12d46elqvinbfbf3xr6/bamf_nnunet_mr_prostate.zip?rlkey=tlcy6mk2cdo1xvjx4fnxful1e&dl=0

Test Results (2024-01-23 13:49:27)
Test Results (24.01.29_13.36.48_9caE8eLF4g)

I did the same test and visually the segmentation alongside the ProstateX t2 scan looks ok.
mhub_bamf_prostate

DICOM data looks properly propagated too.

reference data:
url: https://www.dropbox.com/scl/fi/93iy240cpykav0ni3usih/bamf_nnunet_mr_prostate.zip?rlkey=znl98i1kte0f0m9a8m5acht6q&dl=0

@LennyN95
Copy link
Member

LennyN95 commented Feb 7, 2024

Thx @ccosmin97, I also checked the images and they indeed appear identical upon visual inspection in 3D Slicer.

However, the images provided by the contributor have a different orientation than those generated by us when running the pipeline:

ref_local:  [6] 0.999999225, -0.000398610515, -0.00119197974, -2.07064782e-10, -0.948376417, 0.317146838
src_bamf:   [6] 0.999999166, -0.000406934792, -0.00124311948,  8.32405385e-06, -0.948376417, 0.317146838

Therefore, our checking pipeline fails (as it should). @rahul99 pointed out that they generated the provided images with a different nifti conversion engine (dcm2niix) and changed that later (plastimatch).

We will run a new test once they repeat the test with the actual latest version of their model and then we'll see if all checks pass this time - fingers crossed ;)!

@ccosmin97
Copy link
Contributor

@LennyN95 makes sense, I think its worth adding that the test I ran on my end was after the commit pushing the change to plastimatch engine.

@fedorov
Copy link
Member

fedorov commented Feb 7, 2024

the images provided by the contributor have a different orientation than those generated by us when running the pipeline

@LennyN95 it might be good to add tolerance parameter to your testing. In this case, differences are below 0.001.

@LennyN95
Copy link
Member

LennyN95 commented Feb 8, 2024

Sure, @fedorov, we have tolerances in place, e.g., DiceScore comparison, and we even report the precision of differences in values found within JSON or YAML files. In this specific case, I'm still determining because the idea of the tests is to re-create the same pipeline as the contributor (and using different converters violates this).

@fedorov
Copy link
Member

fedorov commented Feb 8, 2024

I'm still determining because the idea of the tests is to re-create the same pipeline as the contributor (and using different converters violates this).

Yes, I completely agree. And I think that at least to some degree this could be addressed with CI or some sort of automation.

@LennyN95 LennyN95 removed the REQUEST TEST Attach this label to your PR when your submission is ready to be tested by us. label Feb 27, 2024
@rahul99
Copy link
Author

rahul99 commented Mar 13, 2024

Hey @fedorov , circling back on this one to check if any action is needed from our side

@fedorov
Copy link
Member

fedorov commented Mar 13, 2024

@rahul99 it is @LennyN95 who should chime in here!

@github-actions github-actions bot added the INVALID TEST REQUEST The contributor requested a test but the test block is not valid. label Mar 14, 2024
@rahul99
Copy link
Author

rahul99 commented Mar 14, 2024

/test

sample:
  idc_version: 17.0
  data:
  - SeriesInstanceUID: 1.3.6.1.4.1.14519.5.2.1.7311.5101.160028252338004527274326500702
    aws_url: s3://idc-open-data/76d402d3-ea3f-464b-a5f2-f4f7ff813eab/*
    path: dicom

reference:
  url: https://www.dropbox.com/scl/fi/3e26md51q4tcv959euzmv/bamf_nnunet_mr_prostate.zip?rlkey=sz06650fgnrwdtt35cw9s7as7&dl=0

Test Results (24.03.19_11.03.24_fGQob2PznA)
id: 3303bd99-b3ea-43aa-a5d3-1b40ed266728
date: '2024-03-19 11:15:18'
checked_files:
- file: bamf_nnunet_mr_prostate.seg.dcm
  path: /app/test/src/1.3.6.1.4.1.14519.5.2.1.7311.5101.160028252338004527274326500702/bamf_nnunet_mr_prostate.seg.dcm
  checks:
  - checker: DicomsegContentCheck
    notes:
    - label: Segment Count
      description: The number of segments identified in the inspected dicomseg file.
      info: 1
summary:
  files_missing: 0
  files_extra: 0
  checks:
    DicomsegContentCheck:
      files: 1
conclusion: true

@github-actions github-actions bot added TEST REQUESTED and removed INVALID TEST REQUEST The contributor requested a test but the test block is not valid. labels Mar 14, 2024
@LennyN95 LennyN95 merged commit 470da8e into MHubAI:main Mar 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants