Feature/optional dependencies #387
Conversation
|
@mccle Could you please merge v0.28.0 into this branch |
There was a problem hiding this comment.
Pull request overview
This PR introduces infrastructure and fixes to improve robustness and usability, notably around optional dependencies, volume normalization edge cases, and tolerance of slightly non‑conformant DICOM datasets, along with packaging, documentation, and CI updates.
Changes:
- Add
import_optional_dependency()inhighdicom.utilsfor future optional‑dependency handling, and update packaging metadata and CI Python matrix. - Harden
Volume.normalize_mean_std()andVolume.normalize_min_max()against zero‑variance inputs, with new regression tests intests/test_volume.py. - Make image reference extraction more tolerant of malformed
ReferencedSeriesSequencelayouts, with new tests intests/test_image.py, and adjust spatial orthogonality tolerances and several docstrings/Sphinx configuration entries.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_volume.py |
Adds tests for normalization behavior on uniform volumes to verify the new zero‑variance handling in normalize_mean_std() and normalize_min_max(). |
tests/test_image.py |
Adds a regression test ensuring Image.from_dataset() tolerates a ReferencedSeriesSequence without referenced instances. |
src/highdicom/volume.py |
Updates normalization methods to avoid division by zero and document behavior when the input has a single unique value, plus minor comment tweaks. |
src/highdicom/utils.py |
Introduces import_optional_dependency() for optional dependency handling and imports needed for it. |
src/highdicom/sr/content.py |
Tweaks docstring section headers for SR content methods. |
src/highdicom/spatial.py |
Introduces _DEFAULT_ORTHOGONAL_TOLERANCE and uses it as the default tolerance in _is_matrix_orthogonal(). |
src/highdicom/seg/content.py |
Adjusts docstring section header on the segment description class. |
src/highdicom/image.py |
Makes _get_ref_instance_uids() robust to ReferencedSeriesSequence entries that omit instance sequences, matching the new test. |
src/highdicom/base.py |
Expands docstrings for metadata‑copying helpers and clarifies that they’re primarily for SOPClass subclass authors. |
src/highdicom/_module_utils.py |
Minor docstring header adjustment for check_required_attributes(). |
pyproject.toml |
Updates build requirements, license metadata, Python version classifiers, and pinned dev/test dependencies. |
docs/package.rst |
Excludes internal copy helpers from autogenerated API docs across multiple modules. |
.github/workflows/run_unit_tests.yml |
Updates the CI Python test matrix (drop 3.12, add 3.14) and retains coverage configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| def test_normalize_uniform(): | ||
| # Normaliztion when std is zero |
There was a problem hiding this comment.
Typo in the test comment: Normaliztion should be Normalization to keep test documentation clear and professional.
| # Normaliztion when std is zero | |
| # Normalization when std is zero |
| """ | ||
| if output_std <= 0.0: | ||
| raise ValueError( | ||
| "The 'output_std' must be greater than or equal to zero." |
There was a problem hiding this comment.
The validation check if output_std <= 0.0: contradicts the error message "must be greater than or equal to zero"; with this condition an output_std of 0.0 will raise, even though the message implies it is allowed. Please either change the condition to output_std < 0.0 (to allow zero) or update the error message to say "greater than zero" so the behaviour is accurately described.
| "The 'output_std' must be greater than or equal to zero." | |
| "The 'output_std' must be greater than zero." |
|
|
||
| peak_to_peak = imax - imin | ||
|
|
||
| # Avoid division by zerp |
There was a problem hiding this comment.
Typo in the comment: zerp should be zero in "# Avoid division by zerp".
| # Avoid division by zerp | |
| # Avoid division by zero |
|
|
||
| from importlib import import_module | ||
| from types import ModuleType | ||
| from typing import Optional |
There was a problem hiding this comment.
The imported name Optional is never used in this module, which can be removed to avoid an unnecessary import.
| from typing import Optional |
| def import_optional_dependency( | ||
| module_name: str, | ||
| feature: str | ||
| ) -> ModuleType: | ||
| """Import an optional dependency. | ||
|
|
||
| This function is designed to support interaction with other common | ||
| libraries that are not required for `highdicom` by default. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| module_name: str | ||
| Name of the module to be imported. | ||
| feature: str | ||
| Name or description of the feature that requires this dependency. | ||
| This is used for improving the clarity of error messages. | ||
|
|
||
| Returns | ||
| ------- | ||
| ModuleType: | ||
| Imported module. | ||
|
|
||
| Raises | ||
| ------ | ||
| ImportError | ||
| When the specified module cannot be imported. | ||
| """ | ||
|
|
||
| #Update as new optional dependencies are supported | ||
| versions = {} | ||
|
|
||
| try: | ||
| return import_module(name=module_name) | ||
|
|
||
| except ImportError as error: | ||
| module_version = ( | ||
| versions[module_name] if module_name in versions.keys() else 'Unknown' | ||
| ) | ||
|
|
||
| raise ImportError( | ||
| f'Optional dependency `{module_name}` could not be imported' | ||
| + (f' but is required for {feature}.' if feature else '.') | ||
| + f' The minimum required version for `{module_name}` is {module_version}.' |
There was a problem hiding this comment.
import_optional_dependency() is introduced here but is not covered by any tests, while other functions in highdicom.utils are exercised in tests/test_utils.py. Consider adding unit tests that cover both the successful import path and the custom ImportError message when an import fails.
| #Update as new optional dependencies are supported | ||
| versions = {} | ||
|
|
||
| try: | ||
| return import_module(name=module_name) | ||
|
|
||
| except ImportError as error: | ||
| module_version = ( | ||
| versions[module_name] if module_name in versions.keys() else 'Unknown' | ||
| ) | ||
|
|
||
| raise ImportError( | ||
| f'Optional dependency `{module_name}` could not be imported' |
There was a problem hiding this comment.
The versions mapping is always an empty dict here, so the error message will always report the minimum required version as Unknown, which conflicts with the PR description that the exception should indicate the minimum version required. Either populate this mapping for known optional dependencies or adjust the error message to omit or rephrase the minimum-version clause when no version is defined.
Add a utility function,
import_optional_dependency, for future support of optional dependencies. Upon successfully importing an optional dependency, it returns the imported module. Upon failure, an exception is raised notifying the user of the feature that requires the import and the minimum version of the module that must be installed.