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 Liver MR #45

Merged
merged 20 commits into from
Mar 29, 2024
Merged

Conversation

rahul99
Copy link

@rahul99 rahul99 commented Aug 28, 2023

No description provided.

@LennyN95 LennyN95 changed the title resources for bamf-nnunet-mr-liver model BAMF - NNUnet Liver MR Aug 31, 2023
@vanossj
Copy link
Contributor

vanossj commented Nov 8, 2023

Hi @LennyN95, We added the preprint info into the meta.json. This PR should be good to go. Let us know if there is are any updated to the code or meta.json we need to make to keep current with the latest mhub api/requirements

@LennyN95
Copy link
Member

LennyN95 commented Nov 8, 2023

Hi @vanossj, thank you for the update!

I reviewed the model and found the following open issues:

  • You define a BamfProcessorRunner under scripts/BamfProcessor.py, but it's not used in the config.yml. Is this a required post-processing step?
  • The DsegConverter should use the ROI instead of a custom JSON as the ROI is available in our SegDB (read here). You can remove the DsegConverter.json_config_path argument.
  • The config/meta.json can be removed.
  • Remove /scripts as all MHub models are required to provide a default.yml entry point only (we support scripting within our framework but do not allow it in our models repository for standardization purposes)
  • Remove all COPYand ADD instructions from the Dockerfile. We do not allow those.
  • The Dockerfile Entrypoint must be ENTRYPOINT ["python3", "-m", "mhubio.run"]. The CMD -["--config", "/app/path/to/default.yml"] then must point to your default config.

In addition, we will require a public example to verify the model works correctly.

  • Can you list one (or multiple) suitable IDC collections that should be supported by the model?
  • Can you run the MHub implementation of the model on 2-3 example cases, and share the link to the examples with us as well as the generated output (untouched)?

I'm currently working on an automation pipeline. In the near future, sample data will likely be referenced in the meta.json ;)

Thank you for your contribution!

@LennyN95 LennyN95 marked this pull request as ready for review November 9, 2023 16:05
@LennyN95 LennyN95 closed this Nov 9, 2023
@LennyN95 LennyN95 reopened this Nov 9, 2023
models/bamf_nnunet_mr_liver/config/default.yml Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/config/meta.json Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/meta.json Show resolved Hide resolved
models/bamf_nnunet_mr_liver/scripts/BamfProcessor.py Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/scripts/run.py Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/scripts/slicer_run.py Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/utils/BamfProcessorRunner.py Outdated Show resolved Hide resolved
Rahul Soni added 6 commits November 14, 2023 03:18
    Supervised to supervised
- In /summary/model/cmpapproach
    "#D, Emsemble" to "3D"
- /details/licence to /details/license
@fedorov
Copy link
Member

fedorov commented Dec 20, 2023

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

*SeriesInstanceUID

  • aws_url for the bucket folder 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.

models/bamf_nnunet_mr_liver/config/default.yml Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/config/default.yml Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/config/default.yml Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/config/default.yml Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/config/default.yml Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/scripts/slicer_run.py Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/utils/BamfProcessorRunner.py Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/utils/BamfProcessorRunner.py Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/utils/BamfProcessorRunner.py Outdated Show resolved Hide resolved
@LennyN95 LennyN95 mentioned this pull request Jan 8, 2024
@rahul99
Copy link
Author

rahul99 commented Mar 13, 2024

/test

sample:
  idc_version: 17.0
  data:
  - SeriesInstanceUID: 1.3.6.1.4.1.14519.5.2.1.3344.4008.993129740958163005437786531631
    aws_url: s3://idc-open-data/124ed9dd-2085-4d8f-99aa-76b9430b2b95/*
    path: dicom

reference:
  url: https://www.dropbox.com/scl/fi/xbt99t9618aii0hv1jcwx/bamf_nnunet_mr_liver.zip?rlkey=ai8jrin43ce43wxsf4iresqv4&dl=0

Test Results (24.03.28_17.30.31_kb1YE6N1mX)
id: 301ae31d-56e0-43c8-b759-5700478cbe52
date: '2024-03-28 17:59:25'
checked_files:
- file: bamf_nnunet_mr_liver.seg.dcm
  path: /app/test/src/1.3.6.1.4.1.14519.5.2.1.3344.4008.993129740958163005437786531631/bamf_nnunet_mr_liver.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 the INVALID TEST REQUEST The contributor requested a test but the test block is not valid. label Mar 13, 2024
@MHubAI MHubAI deleted a comment from rahul99 Mar 14, 2024
@MHubAI MHubAI deleted a comment from rahul99 Mar 14, 2024
@fedorov
Copy link
Member

fedorov commented Mar 14, 2024

IMHO, new test entry on modification is a lot more transparent.

Further, (and fortunately!) it is not always possible to edit comments of other users.

But I am not the contributor, so you guys proceed as it works for you.

@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
Copy link
Member

I agree with all modifications @fedorov, especially if a test has been performed already. However, for correction of mistakes, it is too much to have 2,3,4.. of these information-dense blocks because you have to find the one used in testing then carefully. If a different user proposes a different test, that's fine. I just meant small changes like a wrong key or bad formatting. So, the rule of thumb could be that you should edit your test whenever the "INVALID TEST REQUEST" label is assigned. Otherwise, create a new one.

models/bamf_nnunet_mr_liver/meta.json Show resolved Hide resolved
models/bamf_nnunet_mr_liver/meta.json Outdated Show resolved Hide resolved
models/bamf_nnunet_mr_liver/meta.json Outdated Show resolved Hide resolved
@LennyN95 LennyN95 merged commit bebc70e into MHubAI:main Mar 29, 2024
1 check passed
@LennyN95
Copy link
Member

@rahul99 could you upload 1-5 images for the model card that show the model in action? This is not a hard requirement, thus I moved on with the push. However, from a UX perspective, visuals add a great value to the model page and also help readers to quickly recognize a model (especially as the model card is made to look similar across all models, images can be a great help here).

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