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

Add new extension for automated 3D shape classification (Duplicate) #111

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

Conversation

allemangD
Copy link
Contributor

@allemangD allemangD commented Jan 10, 2025

This is a duplicate of #109. I attempted to push b6f50ef to lucieDLE:main, so that CMakeLists fix would be added to #109. However, I did not realize the upstream of my branch was DCBIA-OrthoLab, and not lucieDLE. The branch protection rules for DCBIA-OrthoLab:main disabled "Do not allow bypassing the above settings", and since I am an administrator that push immediately merged #109 before I had submitted my review.

I have reverted that merge to main, and updated the branch protection rules so that all changes to main must go through PR (even for administrators) to avoid this in the future.

The original PR description is copied below:


Summary

This PR introduces the DOC-ShapeAXI extension, a new Slicer module developed to automate the classification of 3D shape for dental applications. The extension integrates advanced deep learning models that analyze upper airways, mandibular condyles, and cleft lip and palate to assist clinicians in diagnosis and early detection of conditions.

Key Features

  • Deep-Learning Integration: integrate ShapeAXI for advanced deep-learning classification.
  • User-Friendly interface: Easy-to-use interface for clinicians with minimal technical background.
  • Explainability: Offers insights into model decision-making, enhancing transparency for clinical use.
  • Cross-Platform Compatibility: Compatible with Linux and Windows, supporting usage across various clinical setups.

Testing

  • The extension has been tested for both Linux and Windows distributions with Slicer 5.6

Additional Information

The extension was originally developed at https://github.com/lucieDLE/DOC-ShapeAXI/. A more in-depth description and commit history is avalaible.

A detailed description of the extension, including its usage and prerequisite, has been added to the README of SlicerAutomatedDentalTools.

https://github.com/lucieDLE/SlicerAutomatedDentalTools?tab=readme-ov-file#prerequisites-3.

Copy link
Contributor Author

@allemangD allemangD left a comment

Choose a reason for hiding this comment

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

I suggest formatting the code with black or ruff for consistency. The exact style details don't matter much, but with so many modules with diverging code styles it would be good to get everything into a uniform state.

I would probably wait to do this till after this PR; running the formatter will touch all the python files in the repo and it would be good to have that change isolated to its own PR, rather than mixed in with the new functionality here.

Slicer core uses ruff, you could use the Slicer style as a starting point. https://github.com/Slicer/Slicer/blob/main/.ruff.toml

import threading
import pkg_resources
import signal
sys.path.append('../../../ShapeAXI/')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not modify the path like this. This doesn't look to be used anywhere, so it should be a simple matter of removing this line. If there are any usages of scripts in this path, they should be committed and imported via DOCShapeAXI_utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we move these checkpoints to a repo in the DCBIA-OrthoLab org and update the URLs accordingly?

import sys
import time
import threading
import pkg_resources
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pkg_resources should not be used. It looks like this import is unused anyway, so just remove this line.

Comment on lines 14 to 15
from pathlib import Path
import re
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are unused and can be removed. Although we probably want to be using pathlib instead of manual path manipulation, anyway...

import os
import vtk, qt, slicer
from slicer.ScriptedLoadableModule import *
from slicer.util import VTKObservationMixin, pip_install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pip_install is unused and can be removed. It shouldn't be used directly anyway.

Comment on lines 40 to 52
This extension provides a Graphical User Interface (GUI) for a deep learning automated classification of Alveolar Bone Defect in Cleft, Nasopharynx Airway Obstruction and Mandibular Condyles.

- The input file must be a folder containing a list of vtk files.

- data type for classification: <br>Mandibular Condyle<br>, <br>Nasopharynx Airway Obstruction<br> and <br>Alveolar Bone Defect in Cleft<br>

- output directory: a folder that will contain all the outputs (models, prediction and explainability results)

When prediction is over, you can open the output csv file which will containing the path of each .vtk file as well as the predicted class.
<br><br>

More help can be found on the <a href="https://github.com/DCBIA-OrthoLab/SlicerDentalModelSeg">Github repository</a> for the extension.
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These lines are very long, I suggest wrapping the text around 100 characters wide so this is easier to read.

Comment on lines 369 to 376
text = "WSL doen't have all the necessary libraries, please download the installer and follow the instructions <a href=\"https://github.com/DCBIA-OrthoLab/SlicerAutomatedDentalTools/releases/download/wsl2_windows/installer_WSL2.zip\">here</a> for installation. The link may be blocked by Chrome, just authorize it."

messageBox.information(None, "Information", text)
return False
else : # if wsl not install, ask user to install it ans stop process
messageBox = qt.QMessageBox()
text = "Code can't be launch. \nWSL is not installed, please download the installer and follow the instructin here : https://github.com/DCBIA-OrthoLab/SlicerAutomatedDentalTools/releases/download/wsl2_windows/installer_WSL2.zip may be blocked by Chrome, this is normal, just authorize it."
text = "WSL is not installed, please download the installer and follow the instructions <a href=\"https://github.com/DCBIA-OrthoLab/SlicerAutomatedDentalTools/releases/download/wsl2_windows/installer_WSL2.zip\">here</a> for installation. The link may be blocked by Chrome, just authorize it."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For very long text like this you could either use textwrap.dedent like this. Note this will keep any newlines in the text.

  text = textwrap.dedent("""
    WSL doesn't ...
    and follow ...
    ... authorize it.
  """).strip()

Or you can wrap the text in parentheses like this. Note this is just concatenating the string literals, so any whitespace must be explicit. Note the trailing spaces.

  text = (
    "WSL doesn't ... installer "
    "and follow ... "
    "... authorize it."
  )

Whether you do these or something else, I suggest breaking these strings into shorter lines so the code is easier to read.

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 comment also applies to the other string literals in this file which are more than about 100 characters long.

return 'Ubuntu' in clean_output


class DOCShapeAXILogic(ScriptedLoadableModuleLogic):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic class is empty; I'd move some of the code out of the widget class and into this one.

Generally speaking, methods in the widget should only deal with functionality required to present the UI (enabling/disabling buttons, parsing inputs, updating outputs, etc.) or directly call out to other logics (eg. this class, other Slicer modules, or MRML Node functionality).

Some functions in particular that could be moved:

  • All the subprocess and conda management in condaRunCommand, onProcessUpdate, onProcessCompleted, onCancel, onApplyChangesButton, onCheckRequirements, etc.
  • Related utilities like check_lib_wsl and is_ubuntu_installed.

I'm not saying to move these methods in their entirity; I'm saying to move the "business logic" out of those methods and into new methods of DOCShapeAXILogic. Aim to have clear function arguments so there is no implicit dependence on the particular UI. Doing all this makes it easier to isolate UI changes/bugfixes to the Widget class, and isolate functionality changes/fixes to the Logic class in a composable way.

For example, perhaps DOCShapeAXIWidget.onCancel could be implemented like:

  def onCancel(self):
    self.ui.labelBar.setText(f'Cancelling process...')

    self.logic.cancel_current_task()

    self.cancel = True
    self.ui.cancelButton.setEnabled(False)
    self.removeObservers()  
    self.onReset()

Where DOCShapAXILogic.cancel_current_task handles all the functionality of checking whether there is a current task, cancelling it, and waiting for it to terminate.


from tqdm import tqdm

import shapeaxi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shapeaxi seems unused, this line could be removed.

Comment on lines 22 to 23
from pytorch_grad_cam import GradCAM
from pytorch_grad_cam.utils.model_targets import ClassifierOutputTarget
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GradCAM and ClassifierOutputTarget both seem unused, these could be removed.

@allemangD
Copy link
Contributor Author

@lucieDLE @luciacev see feedback above. Apologies again for the mishap with the original PR #109, I updated the branch protection rule so it should not be possible for direct pushes to main in the future.

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

Successfully merging this pull request may close these issues.

2 participants