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

MHub / GC - Minor cleanup gc_lunglobes #42

Merged
merged 8 commits into from
Mar 4, 2024

Conversation

silvandeleemput
Copy link
Contributor

@silvandeleemput silvandeleemput commented Aug 2, 2023

Closes #69

This PR adds two minor cleanups to the gc_lunglobes model code:

  • My e-mail address has been updated to my new one.
  • The default.yml pipeline has been simplified by removing the unnecessary nifticonverter since now this is supported by default by the DSegConverter as well.

@silvandeleemput silvandeleemput changed the title MHub / GC - Minor cleanup gc_lobesegmentation MHub / GC - Minor cleanup gc_lunglobes Aug 2, 2023
@LennyN95
Copy link
Member

dicomseg_json_path: /app/models/xie2020_lobe_segmentation/config/dseg.json
We no longer need to use the JSON (actually, the file has been removed already);
we've that changes implemented on the patch-models branch, waiting for final testing before we merge into main (soon).

@LennyN95 LennyN95 self-assigned this Aug 13, 2023
@silvandeleemput
Copy link
Contributor Author

I have merged the main branch (which includes the patch-models branch changes) into this branch to resolve the conflicts, this should be good to add now.

@silvandeleemput
Copy link
Contributor Author

Fixed the commit version of the linked model repo at the latest main branch commit found on 2023/09/13

@LennyN95 LennyN95 closed this Nov 9, 2023
@LennyN95 LennyN95 reopened this Nov 9, 2023
@silvandeleemput
Copy link
Contributor Author

@LennyN95 @denbonte What is the current planned line of action for this PR? It appears the meta.json file needs to be updated to the latest JSON schema. I guess this will be something that you guys can do best since you created the mata.json file for this model.

@LennyN95
Copy link
Member

Thx for your status request @silvandeleemput:

  • meta.json will be updated by us for all already implemented models synchronously with our server's backend.
  • Please make sure the PR branch is up-to-date with the main branch (currently 41 commits behind MHubAI:main).
  • The model is currently crashing when more than one GPU is available and this requires explicit GPU selection on machines with >1 GPUs installed on the host machine. This needs to be addressed, as all MHub models are expected to run with the --gpus all option.

@silvandeleemput
Copy link
Contributor Author

Thx for your status request @silvandeleemput:

  • meta.json will be updated by us for all already implemented models synchronously with our server's backend.
  • Please make sure the PR branch is up-to-date with the main branch (currently 41 commits behind MHubAI:main).

That's fine, notify me once you have updated the meta.json

  • The model is currently crashing when more than one GPU is available and this requires explicit GPU selection on machines with >1 GPUs installed on the host machine. This needs to be addressed, as all MHub models are expected to run with the --gpus all option.

I created an issue for this here: #69, please provide the error codes and I'll forward the issue to the repo owner so hopefully he can fix it in the repo. Otherwise, we can easily patch it.

@silvandeleemput
Copy link
Contributor Author

  • meta.json will be updated by us for all already implemented models synchronously with our server's backend.
  • Please make sure the PR branch is up-to-date with the main branch (currently 41 commits behind MHubAI:main).
  • The model is currently crashing when more than one GPU is available and this requires explicit GPU selection on machines with >1 GPUs installed on the host machine. This needs to be addressed, as all MHub models are expected to run with the --gpus all option.

PR branch has been brought up-to-date and the > 1 GPU issue (#69) has been addressed by referring to the new release of the algorithm which includes a fix.

@LennyN95
Copy link
Member

@silvandeleemput I checked the Metadata, and we need some minor information to merge this PR.

  • specify the dates for code and publication (all date formats need to be yyyy-mm-dd)
  • specify true/false under summary.data.public
  • change summary.model.training to supervised

@silvandeleemput
Copy link
Contributor Author

@silvandeleemput I checked the Metadata, and we need some minor information to merge this PR.

  • specify the dates for code and publication (all date formats need to be yyyy-mm-dd)
  • specify true/false under summary.data.public
  • change summary.model.training to supervised

It has been updated.

@LennyN95
Copy link
Member

Thank you @sil, looks like false must not be placed in quotes or is recognized as a string instead of a boolean.

@LennyN95
Copy link
Member

When you're at it @sil, can you also update the slice thickness (see https://github.com/MHubAI/coordination/issues/137)?

@silvandeleemput
Copy link
Contributor Author

Thank you @sil, looks like false must not be placed in quotes or is recognized as a string instead of a boolean.

Of course, fixed!

When you're at it @sil, can you also update the slice thickness (see https://github.com/MHubAI/coordination/issues/137)?

The link for the mentioned issue seems to be broken. For now, I change the slice thickness to 0.75mm in line with the pixel spacing of the training data.

@LennyN95
Copy link
Member

LennyN95 commented Mar 4, 2024

The link for the mentioned issue seems to be broken. For now, I change the slice thickness to 0.75mm in line with the pixel spacing of the training data.

Sorry, @silvandeleemput that is pointing to a private issue repo we used to keep track of things internally; I forgot it wasn't public. But changes are looking good, I'll merge and run the tests. Thanks!

@LennyN95 LennyN95 merged commit dc3698c into MHubAI:main Mar 4, 2024
1 check passed
@LennyN95
Copy link
Member

LennyN95 commented Mar 4, 2024

@silvandeleemput the model didn't provide any output.

I checked what could've went wrong: you removed the NiftiConverter but specified source_segs: nifti:mod=seg.
Solutions are to either remove that line (the default value for DSegConverter.source_segs contains mha, nifti and nrrd) or specify mha as the file type.

I'll fix this and run the build pipeline again. Please always test-run models after any implementational changes ;)

@silvandeleemput silvandeleemput deleted the m-gc-lobesegmentation-xie branch March 5, 2024 13:22
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.

MHub / GC - gc_lunglobes > 1 GPU crash
2 participants