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

Added first working FMCIB model container #73

Merged
merged 15 commits into from
Mar 15, 2024
Merged

Conversation

surajpaib
Copy link
Contributor

No description provided.

Copy link
Member

@LennyN95 LennyN95 left a comment

Choose a reason for hiding this comment

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

Looking good so far! Some comments, let's chat per zoom if anything is unclear or could be improved ;)

models/fmcib/dockerfiles/Dockerfile Outdated Show resolved Hide resolved
models/fmcib/utils/FMCIBRunner.py Outdated Show resolved Hide resolved
models/fmcib/utils/FMCIBRunner.py Outdated Show resolved Hide resolved
with torch.no_grad():
features = model(image)

feature_dict = {f"feature_{idx}": feature for idx, feature in enumerate(features.flatten().tolist())}
Copy link
Member

Choose a reason for hiding this comment

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

Our logical data wrappers could be a good fit here.

Although we can (and should) return the original output, this, besides adding verbosity, allows us to bring the output into every format using e.g. our ReportExporter right from the config.

models/fmcib_radiomics/dockerfiles/Dockerfile Outdated Show resolved Hide resolved
ARG MHUB_MODELS_REPO
# Add pull models repo command here after local testingRUN
RUN buildutils/import_mhub_model.sh fmcib_radiomics ${MHUB_MODELS_REPO}
RUN wget https://zenodo.org/records/10528450/files/model_weights.torch?download=1 -O /app/model_weights.torch
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we choose a distinct file name or place the weights inside a folder to keep us the option to add then later successive models and have all their weights organized. Let's spend a minute thinking about this.

"contrast": true
},
{
"label": "Center of mass",
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't match with the current default workflow but we can address this later and decide which of the two (json / mask) we want to keep as the default. I'd choose whatever is the easiest and most standard one.

Copy link
Member

Choose a reason for hiding this comment

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

I provided an alternative workflow that can start from the JSON file. I've some ideas on how we could provide a workflow starting from Dicom (which should be the default then) that I want to discuss later!

models/fmcib_radiomics/utils/FMCIBRunner.py Outdated Show resolved Hide resolved
@surajpaib surajpaib marked this pull request as ready for review March 7, 2024 05:08
@surajpaib
Copy link
Contributor Author

I tested the default workflow, made some small fixes and it all looks good.

Also tested the slicer workflow on a folder structure that looks like this
image

Using the PatientID folder as the lead for the structure. It all works very well 🎆

For the testing,
This should pull all the DICOM data: s3://idc-open-data-cr/40db0070-9773-413a-8d7e-c91c37c0006b/* and I'll make a release with the centroids.json and the features.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is here for now just to show you what I did with the slicer stuff, we can just restructure this later as you suggest

@surajpaib
Copy link
Contributor Author

Hmm, I'm not too sure how you'd want the format to be for these on the release (since we don't have any DICOM + json workflows yet and I am not too sure how to do that myself without spending a lot of time 😓 )

So I've attached both the features.json and the centroids.json for the IDC DICOM scan that I linked above. Maybe you have an idea of how to do this best?

features.json
centroids.json

@LennyN95
Copy link
Member

LennyN95 commented Mar 7, 2024

That's great! I'll implement the dicom workflow later today!

@surajpaib
Copy link
Contributor Author

@LennyN95 Here is another sample.

Image: s3://idc-open-data-cr/18b83380-0f27-42f2-af7c-65e2d23668ac/*
Centroid:
LUNG1-076_centroids.json
Features:
LUNG1-076_features.json

@LennyN95
Copy link
Member

LennyN95 commented Mar 14, 2024

/test

sample:
  idc_version: 17.0
  data:
  - SeriesInstanceUID: 1.3.6.1.4.1.32722.99.99.298991776521342375010861296712563382046
    aws_url: s3://idc-open-data-cr/40db0070-9773-413a-8d7e-c91c37c0006b/*
    path: dicom
  - url: https://github.com/MHubAI/models/files/14520712/centroids.json
    path: 1.3.6.1.4.1.32722.99.99.298991776521342375010861296712563382046.json

  - SeriesInstanceUID: 1.3.6.1.4.1.32722.99.99.309695689942006699082558764031786785731
    aws_url: s3://idc-open-data-cr/18b83380-0f27-42f2-af7c-65e2d23668ac/*
    path: dicom
  - url: https://github.com/MHubAI/models/files/14528031/LUNG1-076_centroids.json
    path: 1.3.6.1.4.1.32722.99.99.309695689942006699082558764031786785731.json

reference:
  url: https://github.com/MHubAI/models/files/14612917/fmcib_mhub_default_output.zip

Test Results (24.03.15_11.57.01_d91gDb1QPt)
id: dbce25f8-eaa8-411f-a4ec-d56e6e9f7df6
date: '2024-03-15 11:57:56'
checked_files:
- file: features.json
  path: /app/test/src/1.3.6.1.4.1.32722.99.99.309695689942006699082558764031786785731/features.json
  checks:
  - checker: DataFileCheck
    notes:
    - label: Value Match
      description: These keys have identical values
      info: feature_0,feature_1,feature_2,feature_3,feature_4,feature_5,feature_6,feature_7,feature_8,feature_9,feature_10,feature_11,feature_12,feature_13,feature_14,feature_15,feature_16,feature_17,feature_18,feature_19
        (+ 4076 more)
  - checker: SizeCheck
- file: features.json
  path: /app/test/src/1.3.6.1.4.1.32722.99.99.298991776521342375010861296712563382046/features.json
  checks:
  - checker: DataFileCheck
    notes:
    - label: Value Match
      description: These keys have identical values
      info: feature_0,feature_1,feature_2,feature_3,feature_4,feature_5,feature_6,feature_7,feature_8,feature_9,feature_10,feature_11,feature_12,feature_13,feature_14,feature_15,feature_16,feature_17,feature_18,feature_19
        (+ 4076 more)
  - checker: SizeCheck
summary:
  files_missing: 0
  files_extra: 0
  checks:
    DataFileCheck:
      files: 2
    SizeCheck:
      files: 2
conclusion: true

@github-actions github-actions bot added INVALID TEST REQUEST The contributor requested a test but the test block is not valid. 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 1015299 into MHubAI:main Mar 15, 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.

2 participants