From 543ef583cda0e9d1bc3ab888f7e54706e30164c0 Mon Sep 17 00:00:00 2001 From: Shelly Belsky Date: Sun, 24 Nov 2024 12:45:29 -0500 Subject: [PATCH 1/2] ENH: Improve pre-processing error handling for UI user friendliness --- AutoscoperM/AutoscoperM.py | 118 +++++------------------ AutoscoperM/AutoscoperMLib/ErrorUtils.py | 78 +++++++++++++++ AutoscoperM/CMakeLists.txt | 1 + 3 files changed, 101 insertions(+), 96 deletions(-) create mode 100644 AutoscoperM/AutoscoperMLib/ErrorUtils.py diff --git a/AutoscoperM/AutoscoperM.py b/AutoscoperM/AutoscoperM.py index 1a42746..2541b60 100644 --- a/AutoscoperM/AutoscoperM.py +++ b/AutoscoperM/AutoscoperM.py @@ -18,7 +18,7 @@ ) from slicer.util import VTKObservationMixin -from AutoscoperMLib import IO, SubVolumeExtraction +from AutoscoperMLib import IO, ErrorUtils, SubVolumeExtraction # # AutoscoperM @@ -447,7 +447,8 @@ def onGeneratePartialVolumes(self): trackingSubDir = self.ui.trackingSubDir.text modelSubDir = self.ui.modelSubDir.text segmentationNode = self.ui.pv_SegNodeComboBox.currentNode() - if not self.logic.validateInputs( + + ErrorUtils.validateInputs( volumeNode=volumeNode, segmentationNode=segmentationNode, mainOutputDir=mainOutputDir, @@ -455,9 +456,7 @@ def onGeneratePartialVolumes(self): transformSubDir=tfmSubDir, trackingSubDir=trackingSubDir, modelSubDir=modelSubDir, - ): - raise ValueError("Invalid inputs") - return + ) self.logic.createPathsIfNotExists( mainOutputDir, @@ -503,7 +502,7 @@ def onGenerateConfig(self): camCalList = self.ui.camCalList # Validate the inputs - if not self.logic.validateInputs( + ErrorUtils.validateInputs( volumeNode=volumeNode, mainOutputDir=mainOutputDir, configFileName=configFileName, @@ -513,17 +512,13 @@ def onGenerateConfig(self): trialList=trialList, partialVolumeList=partialVolumeList, camCalList=camCalList, - ): - raise ValueError("Invalid inputs") - return - if not self.logic.validatePaths( + ) + ErrorUtils.validatePaths( mainOutputDir=mainOutputDir, tiffDir=os.path.join(mainOutputDir, tiffSubDir), radiographSubDir=os.path.join(mainOutputDir, radiographSubDir), calibDir=os.path.join(mainOutputDir, calibrationSubDir), - ): - raise ValueError("Invalid paths") - return + ) def get_staged_items(listWidget): staged_items = [] @@ -600,8 +595,8 @@ def get_checked_items(listWidget): self.ui.voxelSizeZ.value, ] - # Validate the inputs - if not self.logic.validateInputs( + # Validate the extracted parameters + ErrorUtils.validateInputs( *trialDirs, *partialVolumeFiles, *camCalFiles, @@ -609,8 +604,7 @@ def get_checked_items(listWidget): *volumeFlip, *renderResolution, *voxel_spacing, - ): - raise ValueError("Invalid inputs") + ) # generate the config file IO.generateConfigFile( @@ -639,15 +633,13 @@ def onImportModels(self): volumeNode = self.ui.volumeSelector.currentNode() - if not self.logic.validateInputs(voluemNode=volumeNode): - raise ValueError("Invalid inputs") - return + ErrorUtils.validateInputs(volumeNode=volumeNode) if self.ui.segSTL_loadRadioButton.isChecked(): segmentationFileDir = self.ui.segSTL_modelsDir.currentPath - if not self.logic.validatePaths(segmentationFileDir=segmentationFileDir): - raise ValueError("Invalid paths") - return + + ErrorUtils.validatePaths(segmentationFileDir=segmentationFileDir) + segmentationFiles = glob.glob(os.path.join(segmentationFileDir, "*.*")) segmentationNode = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLSegmentationNode") segmentationNode.CreateDefaultDisplayNodes() @@ -660,7 +652,7 @@ def onImportModels(self): slicer.mrmlScene.RemoveNode(returnedNode) self.ui.progressBar.setValue((idx + 1) / len(segmentationFiles) * 100) else: # Should never happen but just in case - raise Exception("Please select the 'Segmentation From Model' option in order to import models") + raise ValueError("Please select the 'Segmentation From Model' option in order to import models") return slicer.util.messageBox("Success!") @@ -674,9 +666,7 @@ def onSegmentation(self): volumeNode = self.ui.volumeSelector.currentNode() - if not self.logic.validateInputs(voluemNode=volumeNode): - raise ValueError("Invalid inputs") - return + ErrorUtils.validateInputs(volumeNode=volumeNode) if self.ui.segGen_autoRadioButton.isChecked(): currentVolumeNode = volumeNode @@ -702,7 +692,7 @@ def onSegmentation(self): slicer.mrmlScene.RemoveNode(segmentationNode) currentVolumeNode = self.logic.getNextItemInSequence(volumeNode) else: # Should never happen but just in case - raise Exception("Please select the 'Automatic Segmentation' option in order to generate segmentations") + raise ValueError("Please select the 'Automatic Segmentation' option in order to generate segmentations") return slicer.util.messageBox("Success!") @@ -725,7 +715,7 @@ def onLoadPV(self): tfms_scale = glob.glob(os.path.join(mainOutputDir, transformSubDir, "*_scale.tfm")) if len(vols) == 0: - raise Exception("No data found") + raise ValueError("No data found") return if len(vols) != len(tfms_t) != len(tfms_scale): @@ -801,18 +791,14 @@ def populateListFromOutputSubDir(self, listWidget, fileSubDir, itemType="file"): listWidget.clear() mainOutputDir = self.ui.mainOutputSelector.currentPath - if not self.logic.validateInputs( + ErrorUtils.validateInputs( listWidget=listWidget, mainOutputDir=mainOutputDir, fileSubDir=fileSubDir, - ): - raise ValueError("Invalid inputs") - return + ) fileDir = os.path.join(mainOutputDir, fileSubDir) - if not self.logic.validatePaths(fileDir=fileDir): - raise ValueError(f"Invalid input: subdirectory '{fileDir}' does not exist.") - return + ErrorUtils.validatePaths(fileDir=fileDir) if itemType == "file": listFiles = [f.name for f in os.scandir(fileDir) if os.path.isfile(f)] @@ -1181,66 +1167,6 @@ def showVolumeIn3D(volumeNode: slicer.vtkMRMLVolumeNode): logic.UpdateDisplayNodeFromVolumeNode(displayNode, volumeNode) slicer.mrmlScene.RemoveNode(slicer.util.getNode("Volume rendering ROI")) - @staticmethod - def validateInputs(*args: tuple, **kwargs: dict) -> bool: - """ - Validates that the provided inputs are not None. - - :param args: list of inputs to validate - :param kwargs: list of inputs to validate - - :return: True if all inputs are valid, False otherwise - """ - statuses = [] - for arg in args: - status = True - if arg is None: - logging.error(f"{arg} is None") - status = False - if isinstance(arg, str) and arg == "": - logging.error(f"{arg} is an empty string") - status = False - statuses.append(status) - - for name, arg in kwargs.items(): - status = True - if arg is None: - logging.error(f"{name} is None") - status = False - if isinstance(arg, str) and arg == "": - logging.error(f"{name} is an empty string") - status = False - statuses.append(status) - - return all(statuses) - - @staticmethod - def validatePaths(*args: tuple, **kwargs: dict) -> bool: - """ - Checks that the provided paths exist. - - :param args: list of paths to validate - :param kwargs: list of paths to validate - - :return: True if all paths exist, False otherwise - """ - statuses = [] - for arg in args: - status = True - if not os.path.exists(arg): - logging.error(f"{arg} does not exist") - status = False - statuses.append(status) - - for name, path in kwargs.items(): - status = True - if not os.path.exists(path): - logging.error(f"{name} ({path}) does not exist") - status = False - statuses.append(status) - - return all(statuses) - @staticmethod def createPathsIfNotExists(*args: tuple) -> None: """ diff --git a/AutoscoperM/AutoscoperMLib/ErrorUtils.py b/AutoscoperM/AutoscoperMLib/ErrorUtils.py new file mode 100644 index 0000000..2a4bb84 --- /dev/null +++ b/AutoscoperM/AutoscoperMLib/ErrorUtils.py @@ -0,0 +1,78 @@ +import os + + +# +# ValueErrorsException: A custom exception class that accepts a list of errors +# +class ValueErrorsException(Exception): + def __init__(self, errors): + if not isinstance(errors, list): + raise ValueError("The errors input must be a list") + if len(errors) < 1: + raise ValueError("The errors list must contain at least one error") + self.errors = errors + super().__init__("\n".join(errors)) + + def __str__(self): + err_str = "Invalid value" if len(self.errors) == 1 else "Multiple invalid values" + + return err_str + ":\n" + "\n".join(self.errors) + + +# +# helper functions +# + + +def validateInputs(*args: tuple, **kwargs: dict) -> bool: + """ + Validates that the provided inputs are not None or empty. + + :param args: list of inputs to validate + :param kwargs: list of inputs to validate + :raises: ValueErrorsException + + :return: True if all inputs are valid + """ + errors = [] + for arg in args: + if arg is None: + errors.append("Input argument is None") + if isinstance(arg, str) and arg == "": + errors.append("Input argument is an empty string") + + for name, arg in kwargs.items(): + if arg is None: + errors.append(f"Input '{name}' is None") + if isinstance(arg, str) and arg == "": + errors.append(f"Input '{name}' is an empty string") + + if len(errors) != 0: + raise ValueErrorsException(errors) + + return True + + +def validatePaths(*args: tuple, **kwargs: dict) -> bool: + """ + Checks that the provided paths exist. + + :param args: list of paths to validate + :param kwargs: list of paths to validate + :raises: ValueErrorsException + + :return: True if all paths exist + """ + errors = [] + for arg in args: + if not os.path.exists(arg): + errors.append(f"Input path '{arg}' does not exist") + + for name, path in kwargs.items(): + if not os.path.exists(path): + errors.append(f"Input path '{name}' ({path}) does not exist") + + if len(errors) != 0: + raise ValueErrorsException(errors) + + return True diff --git a/AutoscoperM/CMakeLists.txt b/AutoscoperM/CMakeLists.txt index f752650..29d5949 100644 --- a/AutoscoperM/CMakeLists.txt +++ b/AutoscoperM/CMakeLists.txt @@ -5,6 +5,7 @@ set(MODULE_NAME AutoscoperM) set(MODULE_PYTHON_SCRIPTS ${MODULE_NAME}.py ${MODULE_NAME}Lib/__init__.py + ${MODULE_NAME}Lib/ErrorUtils.py ${MODULE_NAME}Lib/IO.py ${MODULE_NAME}Lib/SubVolumeExtraction.py ) From eb3601536afa94de9daaf4cec8fee62e7d1ef522 Mon Sep 17 00:00:00 2001 From: Shelly Belsky Date: Mon, 13 Jan 2025 11:48:33 -0500 Subject: [PATCH 2/2] ENH: Incorporate code review suggestions Co-authored-by: Jean-Christophe Fillion-Robin --- AutoscoperM/AutoscoperM.py | 20 ++++++------- .../{ErrorUtils.py => Validation.py} | 30 +++++-------------- AutoscoperM/CMakeLists.txt | 2 +- 3 files changed, 19 insertions(+), 33 deletions(-) rename AutoscoperM/AutoscoperMLib/{ErrorUtils.py => Validation.py} (75%) diff --git a/AutoscoperM/AutoscoperM.py b/AutoscoperM/AutoscoperM.py index 2541b60..0db2a12 100644 --- a/AutoscoperM/AutoscoperM.py +++ b/AutoscoperM/AutoscoperM.py @@ -18,7 +18,7 @@ ) from slicer.util import VTKObservationMixin -from AutoscoperMLib import IO, ErrorUtils, SubVolumeExtraction +from AutoscoperMLib import IO, SubVolumeExtraction, Validation # # AutoscoperM @@ -448,7 +448,7 @@ def onGeneratePartialVolumes(self): modelSubDir = self.ui.modelSubDir.text segmentationNode = self.ui.pv_SegNodeComboBox.currentNode() - ErrorUtils.validateInputs( + Validation.validateInputs( volumeNode=volumeNode, segmentationNode=segmentationNode, mainOutputDir=mainOutputDir, @@ -502,7 +502,7 @@ def onGenerateConfig(self): camCalList = self.ui.camCalList # Validate the inputs - ErrorUtils.validateInputs( + Validation.validateInputs( volumeNode=volumeNode, mainOutputDir=mainOutputDir, configFileName=configFileName, @@ -513,7 +513,7 @@ def onGenerateConfig(self): partialVolumeList=partialVolumeList, camCalList=camCalList, ) - ErrorUtils.validatePaths( + Validation.validatePaths( mainOutputDir=mainOutputDir, tiffDir=os.path.join(mainOutputDir, tiffSubDir), radiographSubDir=os.path.join(mainOutputDir, radiographSubDir), @@ -596,7 +596,7 @@ def get_checked_items(listWidget): ] # Validate the extracted parameters - ErrorUtils.validateInputs( + Validation.validateInputs( *trialDirs, *partialVolumeFiles, *camCalFiles, @@ -633,12 +633,12 @@ def onImportModels(self): volumeNode = self.ui.volumeSelector.currentNode() - ErrorUtils.validateInputs(volumeNode=volumeNode) + Validation.validateInputs(volumeNode=volumeNode) if self.ui.segSTL_loadRadioButton.isChecked(): segmentationFileDir = self.ui.segSTL_modelsDir.currentPath - ErrorUtils.validatePaths(segmentationFileDir=segmentationFileDir) + Validation.validatePaths(segmentationFileDir=segmentationFileDir) segmentationFiles = glob.glob(os.path.join(segmentationFileDir, "*.*")) segmentationNode = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLSegmentationNode") @@ -666,7 +666,7 @@ def onSegmentation(self): volumeNode = self.ui.volumeSelector.currentNode() - ErrorUtils.validateInputs(volumeNode=volumeNode) + Validation.validateInputs(volumeNode=volumeNode) if self.ui.segGen_autoRadioButton.isChecked(): currentVolumeNode = volumeNode @@ -791,14 +791,14 @@ def populateListFromOutputSubDir(self, listWidget, fileSubDir, itemType="file"): listWidget.clear() mainOutputDir = self.ui.mainOutputSelector.currentPath - ErrorUtils.validateInputs( + Validation.validateInputs( listWidget=listWidget, mainOutputDir=mainOutputDir, fileSubDir=fileSubDir, ) fileDir = os.path.join(mainOutputDir, fileSubDir) - ErrorUtils.validatePaths(fileDir=fileDir) + Validation.validatePaths(fileDir=fileDir) if itemType == "file": listFiles = [f.name for f in os.scandir(fileDir) if os.path.isfile(f)] diff --git a/AutoscoperM/AutoscoperMLib/ErrorUtils.py b/AutoscoperM/AutoscoperMLib/Validation.py similarity index 75% rename from AutoscoperM/AutoscoperMLib/ErrorUtils.py rename to AutoscoperM/AutoscoperMLib/Validation.py index 2a4bb84..1de2a35 100644 --- a/AutoscoperM/AutoscoperMLib/ErrorUtils.py +++ b/AutoscoperM/AutoscoperMLib/Validation.py @@ -1,10 +1,9 @@ import os -# -# ValueErrorsException: A custom exception class that accepts a list of errors -# class ValueErrorsException(Exception): + """A custom exception class that accepts a list of errors.""" + def __init__(self, errors): if not isinstance(errors, list): raise ValueError("The errors input must be a list") @@ -14,25 +13,18 @@ def __init__(self, errors): super().__init__("\n".join(errors)) def __str__(self): - err_str = "Invalid value" if len(self.errors) == 1 else "Multiple invalid values" + err_str = "Invalid value{}".format("" if len(self.errors) == 0 else "s") return err_str + ":\n" + "\n".join(self.errors) -# -# helper functions -# - - -def validateInputs(*args: tuple, **kwargs: dict) -> bool: +def validateInputs(*args: tuple, **kwargs: dict) -> None: """ Validates that the provided inputs are not None or empty. :param args: list of inputs to validate - :param kwargs: list of inputs to validate + :param kwargs: dictionary of inputs to validate :raises: ValueErrorsException - - :return: True if all inputs are valid """ errors = [] for arg in args: @@ -47,21 +39,17 @@ def validateInputs(*args: tuple, **kwargs: dict) -> bool: if isinstance(arg, str) and arg == "": errors.append(f"Input '{name}' is an empty string") - if len(errors) != 0: + if len(errors) > 0: raise ValueErrorsException(errors) - return True - -def validatePaths(*args: tuple, **kwargs: dict) -> bool: +def validatePaths(*args: tuple, **kwargs: dict) -> None: """ Checks that the provided paths exist. :param args: list of paths to validate :param kwargs: list of paths to validate :raises: ValueErrorsException - - :return: True if all paths exist """ errors = [] for arg in args: @@ -72,7 +60,5 @@ def validatePaths(*args: tuple, **kwargs: dict) -> bool: if not os.path.exists(path): errors.append(f"Input path '{name}' ({path}) does not exist") - if len(errors) != 0: + if len(errors) > 0: raise ValueErrorsException(errors) - - return True diff --git a/AutoscoperM/CMakeLists.txt b/AutoscoperM/CMakeLists.txt index 29d5949..f84983a 100644 --- a/AutoscoperM/CMakeLists.txt +++ b/AutoscoperM/CMakeLists.txt @@ -5,9 +5,9 @@ set(MODULE_NAME AutoscoperM) set(MODULE_PYTHON_SCRIPTS ${MODULE_NAME}.py ${MODULE_NAME}Lib/__init__.py - ${MODULE_NAME}Lib/ErrorUtils.py ${MODULE_NAME}Lib/IO.py ${MODULE_NAME}Lib/SubVolumeExtraction.py + ${MODULE_NAME}Lib/Validation.py ) set(MODULE_PYTHON_RESOURCES