Skip to content

Add new extension SlicerHeadCTDeid #2166

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

payabvashlab
Copy link

New extension

Tier 1

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

  • [X ] Repository name is Slicer+ExtensionName (except if the repository that hosts the extension can be also used without Slicer)
  • [X ] 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
  • [X ] Extension description summarizes in 1-2 sentences what the extension is usable (should be understandable for non-experts)
  • [X ] Any known related patents must be mentioned in the extension description.
  • [X ] 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.
  • [X ] 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
  • [X ] 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)
  • [X ] Screenshot URLs (screenshoturls) are correct, contains at least one
  • [X ] 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:
    • [X ] Extension name
    • [X ] Short description: 1-2 sentences, which summarizes what the extension is usable for
    • [X ] At least one nice, informative image, that illustrates what the extension can do. It may be a screenshot.
    • [X ] Description of contained modules: at one sentence for each module
    • [X ] 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:
    • [X ] Click Settings and in repository settings uncheck Wiki, Projects, and Discussions (if they are currently not used).
    • [X ] 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:
    • [X ] Does not include or download binaries from unreliable sources
    • [X ] 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.

Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Looks good to me. @lassoan @jcfr any comments before merging?

@jamesobutler
Copy link
Contributor

xref here is the previous review of this extension. I think Andrey had the most underlying comments about it #2145.

@pieper
Copy link
Member

pieper commented Apr 25, 2025

xref here is the previous review of this extension. I think Andrey had the most underlying comments about it #2145.

Yes, I think those issues have been addressed to the extent needed.

@lassoan
Copy link
Contributor

lassoan commented Apr 25, 2025

There are two issues to fix before the extension can be added to the Extensions Index:

  • The extension build machine cannot build the extension as it is now. Top-level CMakeLists.txt file must be in the repository's root. Module's CMakeLists.txt file is missing. I would recommend to regenerate the extension's files using the ExtensionWizard..
  • Extensions must not install any Python dependencies when the module source file is imported, because that would very significantly slow down Slicer startup and would download and install several large, potentially problematic packages (such as opencv) without user consent. The fix is to call dependencies.setupPythonRequirements(upgrade=True) when it is needed (e.g., when you start processing), not in the file scope.

Small recommended cleanup - remove these irrelevant files from the repository:

  • all .DS_Store files
  • all __pycache__ folders
  • SlicerHeadCTDeid.json (it is stored in the Extensions Index)
  • HeadCTDeid.s4ext (generated dynamically, no need to store it in revision control)

@lassoan
Copy link
Contributor

lassoan commented Apr 25, 2025

xref here is the previous review of this extension. I think Andrey had the most underlying comments about it #2145.

@fedorov has invested his time into providing very valuable feedback. It should be utilized. At a very basic level, we take the feeback into account in that the extension is set to be a Tier 1 (experimental, not endorsed or maintained by Slicer developers). However, it would also make sense to improve the extension documentation to include more information about standard, validated methods + describe specific limitations of the tool in detail. At least include the links and notes that Andrey has already described (not just a blanket "Warning: This tool is a work in progress ...Use at your own risk." disclaimer).

In the future, it would be essential to rework the extension to use a standard anonymization solution as a basis and refine, extend, improve that. The medical image computing community cannot afford to be fragmented - developing, maintaining, supporting hundreds of solutions implemented from scratch for solving the exact same task.

@lassoan
Copy link
Contributor

lassoan commented Apr 25, 2025

xref here is the previous review of this extension. I think Andrey had the most underlying comments about it #2145.

Yes, I think those issues have been addressed to the extent needed.

At least a dedicated documentation section or an issue should be added that summarizes Andrey's concerns and suggestions.

@pieper
Copy link
Member

pieper commented Apr 25, 2025

Sure, @lassoan, yes to all that but it's a lot to expect from someone just trying to provide share their helpful research code. Everything you said about duplication and incomplete context is true of segmentation, registration, and other research tools have have arguably a greater chance of harmful misuse.

The current readme is much clearer based on the feedback from the review process and includes references that go into the gory detail of deidentification. I don't know that it all needs to be rehashed in this extension's readme.

@payabvashlab how about adding a link to the discussion thread in the earlier PR along with a summary of the concerns.

Also please address the build and runtime issues that Andras pointed out.

@lassoan
Copy link
Contributor

lassoan commented Apr 25, 2025

Adding a link to the discussion in the PR could be sufficient.

@jamesobutler
Copy link
Contributor

@payabvashlab
Copy link
Author

Thank you all,
Working on making changes.

@lassoan and @pieper , we actually added a warning popup that every time someone applies the tool , it will warn that this is a research tool
We will add some additional documentation in the README file about the use.
Unfortunately, there is still no universal / standardized agreement on what HPI/PPI need to be removed from DICOM header but we are transparent and very much open to any feedback about DICOM tags that we removed - we listed the ones we removed in the PDF document.
Facial recognition add one layer of complexity to de-identification of brain scans and that is what we tried to tackle and it was the very specific need of AHA re head CTs which are the de facto line of acute stroke imaging. de-identification of brain MRIs is much more challenging due to difference in sequences.

In addition to linking to PR discussion, we will include more of this discussion

@jamesobutler
We changed the name of the application, the SlicerDeid is obsolete and we will delete that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants