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

ENH: Add PKDIA extension #2134

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jo-Byr
Copy link

@Jo-Byr Jo-Byr commented Dec 17, 2024

New extension

Tier 1

Any extension that is listed in the Extensions Catalog must fulfill these requirements.

  • Repository name is Slicer+ExtensionName (except if the repository that hosts the extension can be also used without Slicer)
  • Repository is associated with 3d-slicer-extension GitHub topic so that it is listed here. To edit topics, click the settings icon in the right side of "About" section header and enter 3d-slicer-extension in "Topics" and click "Save changes". To learn more about topics, read https://help.github.com/en/articles/about-topics
  • Extension description summarizes in 1-2 sentences what the extension is usable (should be understandable for non-experts)
  • Any known related patents must be mentioned in the extension description.
  • LICENSE.txt is present in the repository root and the name of the license is mentioned in extension homepage. We suggest you use a permissive license that includes patent and contribution clauses. This will help protect developers and ensure the code remains freely available. MIT (https://choosealicense.com/licenses/mit/) or Apache (https://choosealicense.com/licenses/apache-2.0/) license is recommended. Read here to learn more about licenses. If source code license is more restrictive for users than MIT, BSD, Apache, or 3D Slicer license then describe the reason for the license choice and include the name of the used license in the extension description.
  • Extension URL and revision (scmurl, scmrevision) is correct, consider using a branch name (main, release, ...) instead of a specific git hash to avoid re-submitting pull request whenever the extension is updated
  • Extension icon URL is correct (do not use the icon's webpage but the raw data download URL that you get from the download button - it should look something like this: https://raw.githubusercontent.com/user/repo/main/SomeIcon.png)
  • Screenshot URLs (screenshoturls) are correct, contains at least one
  • Content of submitted json file is consistent with the top-level CMakeLists.txt file in the repository (dependencies, etc. are the same)
  • Homepage URL points to valid webpage containing the following:
    • Extension name
    • Short description: 1-2 sentences, which summarizes what the extension is usable for
    • At least one nice, informative image, that illustrates what the extension can do. It may be a screenshot.
    • Description of contained modules: at one sentence for each module
    • Publication: link to publication and/or to PubMed reference (if available)
  • Hide unused github features (such as Wiki, Projects, and Discussions, Releases, Packages) in the repository to reduce noise/irrelevant information:
    • Click Settings and in repository settings uncheck Wiki, Projects, and Discussions (if they are currently not used).
    • Click the settings icon next to About in the top-right corner of the repository main page and uncheck Releases and Packages (if they are currently not used)
  • The extension is safe:
    • Does not include or download binaries from unreliable sources
    • Does not send any information anywhere without user consent (explicit opt-in is required)

Tier 3

Community-supported extensions.

  • Extension has a reasonable name (not too general, not too narrow, suggests what the extension is for)
  • Documentation, tutorial, and test data are provided for most modules. A tutorial provides step-by-step description of at least the most typical use case, include a few screenshots. Any sample data sets that is used in tutorials must be registered with the Sample Data module to provide easy access to the user.
  • Follows programming and user interface conventions of 3D Slicer (e.g., GUI and logic are separated, usage of popups is minimized, no unnecessary custom GUI styling, etc.)
  • The extension can be successfully built and packaged on all supported platforms (Windows, macOS, Linux)
  • Maintainers respond to issues and pull request submitted to the extension's repository.
  • Maintainers respond to questions directly addressed to him/her via @mention on the Slicer Forum.
  • Permissive license is used for the main functions of the extension (recommended Apache or MIT). The extension can provide additional functionality in optional components that are distributed with non-permissive license, but the user has to explicitly approve those before using them (e.g., a pop-up can be displayed that explains the licensing terms and the user has to acknowledge them to proceed).
  • All requirements of tiers < 3.

Tier 5

Critically important extensions, supported by Slicer core developers. New Slicer Stable Release is released only if all Tier 5 extension packages are successfully created on all supported platforms.

  • Slicer core developers accept the responsibility of fixing any issues caused by Slicer core changes; at least one Slicer core developer (anyone who has commit right to Slicer core) must be granted commit right to the extension's repository.
  • Automated tests for all critical features.
  • Maintainers respond to questions related to the extension on the Slicer Forum.
  • All requirements of tiers < 5.

@Jo-Byr Jo-Byr force-pushed the add-slicer-pkdia-extension branch from 53f0b39 to 3122b25 Compare January 7, 2025 14:11
@finetjul
Copy link
Member

finetjul commented Feb 7, 2025

@lassoan @jcfr is there something we forgot in the procedure to get the extension reviewed/merged ?

@lassoan
Copy link
Contributor

lassoan commented Feb 8, 2025

Sorry for the delay, holidays and the Project Week made a the last couple of weeks very busy. I have a couple of comments, I'll post them by Monday.

@lassoan
Copy link
Contributor

lassoan commented Feb 10, 2025

Thank you for your contribution @Jo-Byr et al, very nice work! There are just a few things that would need to be considered before merging the pull request, just because it would be difficult to change it later, and a few more to make the module more successful and impactful.

Before merge:

  • Extension name: It is good that it is short and quite specific, but most people would have no idea what PKDIA stands for and it is hard to remember. Many users would not discover it or could not find it or would not recommend it because they don't remember the exact letter combination. It is also unfortunate that the google hits for PKDIA are in the medical domain but something completely different. Including "kidney" and "segmentation" in the name would help a lot. You could consider names like KidneySegPKDIA or PKDIAKidneySeg or maybe PolycysticKidneySeg. Or, if you are ready to add or accept other kidney segmentation modules into this extension in the future then it could be just KidneySeg.
  • Module name: In general, we avoid starting each module name with "Slicer", because it does not add any useful information, just makes the module name longer. You could use the same name as the extension as module name.
  • All the files that you want to be included in the package must be listed in SlicerPKDIA\SlicerPKDIA\CMakeLists.txt. Currently, all the files in the SlicerPKDIALib subfolder are missed, therefore the module would not load into Slicer when installed via the Extensions Manager. It is tempting to add all files in the subfolder using globbing, but it would result in CMake not knowing exactly when a package needs to be rebuilt (e.g., if you add a file to the SlicerPKDIALib subfolder and rebuild the package, CMake would miss the new file). So, I would recommend to list each file.

Not required to fix before merging, but it would make your life easier (more user would be able to successfully use your extension):

  • I got the error FileNotFoundError: [Errno 2] No such file or directory: '5: AXIAL T2 SSFSE BH-prod.nii.gz'. This is because node names may be too long and contain invalid characters for filename (in this case :). Images loaded from DICOM usually start with the series number followed by :, so many users would run into this issue. It would be simpler to use a hardcoded filename, but if that is not desirable then you can use qSlicerCoreIOManager.forceFileNameValidCharacters() and qSlicerCoreIOManager.forceFileNameMaxLengthExtension() methods to make the node name suitable as a filename.
  • Add link to at least one data set that this module is known to work well on. I've tried it on a random T2 kidney MRI from TCIA (series instance uid: 1.3.6.1.4.1.14519.5.2.1.867044007602064428393129841040266430) and it did not work well. Ideally, register in the SampleData module, as shown in the module template. You can upload a file to your github repository as a release asset if you don't want to rely on an additional site for hosting your sample/test data sets. I've tried the CT segmentation on the Panoramix-cropped data set and the result was not as good as with TotalSegmentator and other models. It is important to provide examples that highlight cases where well-established models like TotalSegmentator don't work that well (I guess TotalSegmentator would struggle segmenting a kidney with severe PKD, while PKDIA may be able to deal with it).
  • If dependencies are not installed then this is all the output the user sees: Start ***... Loading inference results.... After this, the Install dependencies button is disabled, so the user cannot install the dependencies (need to restart Slicer). It would be better to at least catch the exception and show a more informative error message. It would be even better if added a little code snippet that detects that not all requirements are installed and ask permission from the user to install them now. After clicking on installing dependencies, pytorch was not found. It asked me to restart Slicer, I restarted, but maybe then I did not click "install dependencies" button again. The procedure is a bit fragile. It is unfortunately a complex topic, maybe just adding a note to the documentation on how to install pytorch if all else fails could be a workaround.

@jcfr jcfr added the Status: Awaiting Response ⏳ Waiting for a response/more information label Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting Response ⏳ Waiting for a response/more information
Development

Successfully merging this pull request may close these issues.

4 participants