DCBIA-OrthoLab / SlicerAutomatedDentalTools

A 3D Slicer extension to use AMASSS, ALI-CBCT and ALI-IOS
Other
79 stars 22 forks source link

BUG: add option to choose host memory #75

Closed Jeanneclre closed 5 months ago

Jeanneclre commented 5 months ago

Hello @allemangD

I did some changes we discussed in the issue #66. To resume them:

  1. Changed this https://github.com/DCBIA-OrthoLab/SlicerAutomatedDentalTools/blob/86026595834472b132aca980b5cc0688359993c3/AMASSS_CLI/AMASSS_CLI.py#L976

with this:

DEVICE = torch.device("cuda" if torch.cuda.is_available() else "cpu")
val_outputs = sliding_window_inference(input_img, cropSize, args["nbr_GPU_worker"], net,
                                       overlap=args["precision"], sw_device= DEVICE, device=device_memory)                                                 
  1. Added a checkbox on the User Interface of Amass to enable "CPU memory"
  2. Error message in the shell asking user to try to enable CPU as a memory host (couldn't do a pop-up window from the CLI)

Output test: On a machine equipped with an Intel Xeon Gold 6226R CPU running at 2.90GHz, with 62GB of memory and an NVIDIA RTX A6000 48GB, a CBCT scan with an approximate resolution of 0.3mm, covering the maxilla, mandible, skin, and cranial base, was segmented in 126.25 seconds.

Thank you!

tschreiner commented 5 months ago

Cuda fix looks good imho

allemangD commented 5 months ago

Generally the changes look good. I don't think the phrasing of the UI is understandable, though, "CPU usage - check to enable" to me reads as it would execute segmentation on the GPU, I think it may confuse users.

I'd change to something like "Use CPU memory" with a tooltip "Use host memory with a performance penalty. Enable if CUDA runs out of memory." so a user could more easily find it after seeing an error about memory usage.

Jeanneclre commented 5 months ago

Thank you for the feedback! I've made the changes. Please let me know if you have any other suggestions. :)

Jeanneclre commented 5 months ago

Hi @allemangD I added a function to install the required libraries and their version for AMASS. Should we change the PR title?

Thank you