From 2c8e1819b5f136a3405de180bb1cadd799b802e9 Mon Sep 17 00:00:00 2001 From: Stephen Aylward Date: Thu, 27 Aug 2026 11:34:40 -0400 Subject: [PATCH 1/2] BUG: Address nightly failures due to stale/old tests --- .github/workflows/nightly-health.yml | 9 +- .../segment_chest_total_segmentator.py | 33 ++++++- src/physiotwin4d/segment_heart_simpleware.py | 8 +- ...rkflow_fit_statistical_model_to_patient.py | 20 ++++- tests/test_register_time_series_images.py | 8 +- tests/test_segment_chest_total_segmentator.py | 61 ++++++++++++- tests/test_segment_heart_simpleware.py | 90 +++++++++++-------- tests/test_workflow_convert_image_to_usd.py | 8 +- ...rkflow_fit_statistical_model_to_patient.py | 62 +++++++++++++ tutorials/parameters_lung_ct_dirlab.py | 10 +++ ...torial_06_lung_create_statistical_model.py | 19 +++- 11 files changed, 274 insertions(+), 54 deletions(-) diff --git a/.github/workflows/nightly-health.yml b/.github/workflows/nightly-health.yml index 4806608..76fb016 100644 --- a/.github/workflows/nightly-health.yml +++ b/.github/workflows/nightly-health.yml @@ -205,12 +205,19 @@ jobs: # # --cov-append adds to the core run's data, and the reports are written # here so that they cover both runs. + # + # --timeout sits well above --max-test-seconds on purpose. Killing a + # worker discards everything the tutorial had printed, which is exactly + # the record needed to see where its time went, so the backstop must + # only catch a tutorial that will never finish. An overrun that does + # finish is reported by --max-test-seconds instead, with its duration + # and its output intact. run: | pytest tests/test_tutorials.py -v ` --run-all --require-tutorial-data ` -n 1 --max-worker-restart=40 ` --max-test-seconds=600 ` - --timeout=900 ` + --timeout=2400 ` --cov=physiotwin4d ` --cov-append ` --cov-report=xml ` diff --git a/src/physiotwin4d/segment_chest_total_segmentator.py b/src/physiotwin4d/segment_chest_total_segmentator.py index 1cb032b..9dbf224 100644 --- a/src/physiotwin4d/segment_chest_total_segmentator.py +++ b/src/physiotwin4d/segment_chest_total_segmentator.py @@ -44,7 +44,7 @@ class SegmentChestTotalSegmentator(SegmentAnatomyBase): group. Attributes: - target_spacing (float): Target spacing set to 1.5mm for TotalSegmentator. + target_spacing (float): Target spacing set to 1.0mm for TotalSegmentator. Example: >>> segmenter = SegmentChestTotalSegmentator() @@ -225,12 +225,41 @@ def __init__(self, log_level: int | str = logging.INFO): self.has_academic_license = False + @staticmethod + def _academic_license_is_valid() -> bool: + """Return True when TotalSegmentator reports an installed license.""" + from totalsegmentator.libs import ( # noqa: PLC0415 + has_valid_license_offline, + ) + + status, _ = has_valid_license_offline() + return bool(status == "yes") + def set_has_academic_license(self, has_academic_license: bool) -> None: - """Set whether the academic license is available. + """Request the licensed tasks, if a license is actually installed. + + ``heartchambers_highres`` and ``tissue_4_types`` are not openly + available. Asking for them without a license makes + ``totalsegmentator`` print its licensing notice and call + ``sys.exit(1)`` from inside the segmentation, which surfaces as a bare + ``SystemExit`` partway through whatever workflow was running. Check + here instead, so that a machine without a key segments the heart as + one structure rather than aborting the run. The fallback is logged, + because it is a coarser segmentation than a licensed run produces. Args: has_academic_license (bool): Whether the academic license is available """ + if has_academic_license and not self._academic_license_is_valid(): + self.log_warning( + "No valid TotalSegmentator license found; skipping the " + "'heartchambers_highres' and 'tissue_4_types' tasks, so the " + "heart is segmented as a single structure and no chamber " + "labels (141-144) are produced. Install one with " + "'totalseg_set_license -l '; a free academic license is " + "at https://backend.totalsegmentator.com/license-academic/" + ) + has_academic_license = False self.has_academic_license = has_academic_license def _add_extra_taxonomy_groups(self) -> None: diff --git a/src/physiotwin4d/segment_heart_simpleware.py b/src/physiotwin4d/segment_heart_simpleware.py index b73f2d9..cff5eaf 100644 --- a/src/physiotwin4d/segment_heart_simpleware.py +++ b/src/physiotwin4d/segment_heart_simpleware.py @@ -75,10 +75,10 @@ def __init__(self, log_level: int | str = logging.INFO): self.target_spacing = 1.0 # Heart and major-vessel labels from Simpleware Medical ASCardio. - # Lung / bone / soft_tissue are not segmented by ASCardio; they will - # be folded into the 'other' group by _finalize_other_group(). - # Contrast (135) and soft_tissue (133) defaults are inherited from - # SegmentAnatomyBase. + # Lung, bone, soft_tissue and contrast are not segmented by ASCardio, + # and SegmentAnatomyBase seeds no defaults for them, so those groups + # stay empty and never reach a result. Ids ASCardio does emit but no + # group claims are folded into 'other' by _finalize_other_group(). for group_name, organs in ( ( "heart", diff --git a/src/physiotwin4d/workflow_fit_statistical_model_to_patient.py b/src/physiotwin4d/workflow_fit_statistical_model_to_patient.py index e3bc3ff..f454c26 100644 --- a/src/physiotwin4d/workflow_fit_statistical_model_to_patient.py +++ b/src/physiotwin4d/workflow_fit_statistical_model_to_patient.py @@ -390,7 +390,9 @@ def set_use_pca_registration( WorkflowCreateStatisticalModel result["pca_model"]) with keys "eigenvalues" and "components". number_of_pca_components: Required when use is True. Number of PCA - components to use. Default 0 means use all components. + components to use. Default 0 means use all components. A count + larger than the model actually carries is reduced to what it + carries; see below. use_surface: Whether to use the surface of the patient model for PCA registration. Raises: ValueError: If use is True and pca_model is None. @@ -400,6 +402,22 @@ def set_use_pca_registration( raise ValueError( "When enabling PCA registration, pca_model must be provided." ) + # A PCA model carries at most one fewer mode than it had samples, + # and WorkflowCreateStatisticalModel already caps it there. The + # count configured for a full population is therefore too large for + # a model built from a small one, and asking for more modes than + # exist raises out of the optimizer partway through the fit. Read + # the count the model actually carries instead. + available_components = len(pca_model.get("components", [])) + if 0 < available_components < number_of_pca_components: + self.log_info( + "PCA model carries %d mode(s), fewer than the %d requested; " + "fitting with the %d available.", + available_components, + number_of_pca_components, + available_components, + ) + number_of_pca_components = available_components self.pca_model = pca_model self.number_of_pca_components = number_of_pca_components else: diff --git a/tests/test_register_time_series_images.py b/tests/test_register_time_series_images.py index 10396c4..dc43cb4 100644 --- a/tests/test_register_time_series_images.py +++ b/tests/test_register_time_series_images.py @@ -29,13 +29,13 @@ class TestRegisterTimeSeriesImages: _class_name = "registration_time_series_images" def test_registrar_initialization_default(self) -> None: - """Test that the default registration_method is RegisterImagesGreedyICON.""" + """Test that the default registration_method is RegisterImagesGreedy.""" registrar = RegisterTimeSeriesImages() - assert isinstance(registrar.registrar, RegisterImagesGreedyICON), ( - "Default registrar should be RegisterImagesGreedyICON" + assert isinstance(registrar.registrar, RegisterImagesGreedy), ( + "Default registrar should be RegisterImagesGreedy" ) - print("\nTime series registrar defaults to RegisterImagesGreedyICON") + print("\nTime series registrar defaults to RegisterImagesGreedy") def test_registrar_initialization_Greedy_ICON(self) -> None: """Initializes correctly with a RegisterImagesGreedyICON instance.""" diff --git a/tests/test_segment_chest_total_segmentator.py b/tests/test_segment_chest_total_segmentator.py index 804bcb1..e27778f 100644 --- a/tests/test_segment_chest_total_segmentator.py +++ b/tests/test_segment_chest_total_segmentator.py @@ -28,7 +28,7 @@ def test_segmenter_initialization( ) -> None: """Test that SegmentChestTotalSegmentator initializes correctly.""" assert segmenter_total_segmentator is not None, "Segmenter not initialized" - assert segmenter_total_segmentator.target_spacing == 1.5, ( + assert segmenter_total_segmentator.target_spacing == 1.0, ( "Target spacing not set correctly" ) @@ -271,3 +271,62 @@ def test_postprocessing( if __name__ == "__main__": pytest.main([__file__, "-v", "-s"]) + + +def test_academic_license_request_is_honoured_when_a_license_is_installed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """With a license present, the licensed tasks stay requested.""" + # staticmethod(), because setattr on the class would otherwise install a + # plain function and the call would pass self into it. + monkeypatch.setattr( + SegmentChestTotalSegmentator, + "_academic_license_is_valid", + staticmethod(lambda: True), + ) + segmenter = SegmentChestTotalSegmentator() + + segmenter.set_has_academic_license(True) + + assert segmenter.has_academic_license is True + + +def test_academic_license_request_falls_back_when_no_license_is_installed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Without a license, asking for the licensed tasks must not abort the run. + + ``heartchambers_highres`` and ``tissue_4_types`` are not openly available. + Requesting them unlicensed makes ``totalsegmentator`` call ``sys.exit(1)`` + from inside the segmentation, which surfaces as a bare ``SystemExit`` + partway through whatever workflow was running. The request is dropped + here instead, so the heart is segmented as one structure. + """ + monkeypatch.setattr( + SegmentChestTotalSegmentator, + "_academic_license_is_valid", + staticmethod(lambda: False), + ) + segmenter = SegmentChestTotalSegmentator() + + segmenter.set_has_academic_license(True) + + assert segmenter.has_academic_license is False + + +def test_declining_the_academic_license_never_checks_for_one( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Only a request for the licensed tasks should cost a license lookup.""" + + def _fail() -> bool: + raise AssertionError("license checked when it was not requested") + + monkeypatch.setattr( + SegmentChestTotalSegmentator, "_academic_license_is_valid", staticmethod(_fail) + ) + segmenter = SegmentChestTotalSegmentator() + + segmenter.set_has_academic_license(False) + + assert segmenter.has_academic_license is False diff --git a/tests/test_segment_heart_simpleware.py b/tests/test_segment_heart_simpleware.py index f7c3f06..19740c1 100644 --- a/tests/test_segment_heart_simpleware.py +++ b/tests/test_segment_heart_simpleware.py @@ -47,13 +47,16 @@ def test_segmenter_initialization( assert len(taxonomy.labels_in_group("major_vessels")) > 0, ( "Major vessels mask IDs not defined" ) - # ASCardio does not segment lung or bone — those groups are never - # registered, so labels_in_group returns an empty dict for them. - # soft_tissue still contains the base-class placeholder (id 133). + # ASCardio segments neither lung, bone, soft tissue nor contrast, and + # SegmentAnatomyBase seeds no defaults, so labels_in_group returns an + # empty dict for each of them. assert taxonomy.labels_in_group("lung") == {}, "ASCardio does not segment lungs" assert taxonomy.labels_in_group("bone") == {}, "ASCardio does not segment bone" - assert taxonomy.labels_in_group("soft_tissue") == {133: "soft_tissue"}, ( - "Only the base-class soft_tissue placeholder should be present" + assert taxonomy.labels_in_group("soft_tissue") == {}, ( + "ASCardio does not segment soft tissue" + ) + assert taxonomy.labels_in_group("contrast") == {}, ( + "ASCardio does not detect contrast" ) assert seg.simpleware_exe_path is not None, "Simpleware executable path not set" @@ -101,27 +104,24 @@ def test_segment_single_image( assert isinstance(result, dict), "Result should be a dictionary" # The Simpleware segmenter only registers the groups it actually - # populates: heart + major_vessels (subclass) and soft_tissue + - # contrast (inherited base-class placeholders). lung and bone are - # NOT in the result because ASCardio does not segment them; callers - # that need those groups must check membership first. + # populates: heart and major_vessels from the subclass, plus the + # "other" group that collects every unclaimed id. lung, bone, + # soft_tissue and contrast are NOT in the result, because ASCardio + # does not produce them and SegmentAnatomyBase seeds no placeholder + # for them; callers that need those groups must check membership. expected_keys = [ "labelmap", "heart", "major_vessels", - "soft_tissue", - "contrast", "other", ] for key in expected_keys: assert key in result, f"Missing key '{key}' in result" assert result[key] is not None, f"Result['{key}'] is None" - assert "lung" not in result, ( - "ASCardio does not segment lung; key must be absent" - ) - assert "bone" not in result, ( - "ASCardio does not segment bone; key must be absent" - ) + for absent in ("lung", "bone", "soft_tissue", "contrast"): + assert absent not in result, ( + f"ASCardio does not produce {absent}; key must be absent" + ) labelmap = result["labelmap"] assert itk.size(labelmap) == itk.size(input_image), "Labelmap size mismatch" @@ -142,12 +142,17 @@ def test_segment_single_image( ) print(f" Saved to: {seg_output_dir / 'heart_labelmap_simpleware.nii.gz'}") - def test_anatomy_group_masks( + def test_anatomy_group_labelmaps( self, segmenter_simpleware: SegmentHeartSimpleware, test_images: list[Any], ) -> None: - """Test that anatomy group masks are created (heart, vessels, etc.).""" + """Test that anatomy group labelmaps are created (heart, vessels, etc.). + + A group entry is a labelmap carrying that group's own label ids, not a + binary mask, so the ids are checked against the taxonomy rather than + the value count. + """ if not _simpleware_available(segmenter_simpleware): pytest.skip("Simpleware Medical not found. Install to run this test.") @@ -158,42 +163,55 @@ def test_anatomy_group_masks( anatomy_groups = [ "heart", "major_vessels", - "soft_tissue", "other", ] + taxonomy = segmenter_simpleware.taxonomy + for group in anatomy_groups: - assert group in result, f"{group} mask should be present" - mask = result[group] - assert mask is not None, f"{group} mask is None" - mask_arr = itk.array_from_image(mask) - unique_values = np.unique(mask_arr) - assert len(unique_values) <= 2, f"{group} mask should be binary" - assert 0 in unique_values or mask_arr.size == 0 - assert itk.size(mask) == itk.size(input_image), ( - f"{group} mask size mismatch" + assert group in result, f"{group} labelmap should be present" + group_labelmap = result[group] + assert group_labelmap is not None, f"{group} labelmap is None" + + group_labelmap_arr = itk.array_from_image(group_labelmap) + unique_values = set(np.unique(group_labelmap_arr).tolist()) + assert 0 in unique_values, f"{group} labelmap should contain background" + + # "other" collects whatever ids no group claimed, so it has no + # fixed id set to check against. + if group != "other": + allowed_values = {0} | set(taxonomy.labels_in_group(group).keys()) + assert unique_values <= allowed_values, ( + f"{group} labelmap contains unexpected label ids: " + f"{unique_values - allowed_values}" + ) + + assert itk.size(group_labelmap) == itk.size(input_image), ( + f"{group} labelmap size mismatch" ) heart_arr = itk.array_from_image(result["heart"]) vessels_arr = itk.array_from_image(result["major_vessels"]) - print("\nAll anatomy group masks created correctly") + print("ANATOMY GROUP LABELMAPS") print(f" heart: {np.sum(heart_arr > 0)} voxels") print(f" major_vessels: {np.sum(vessels_arr > 0)} voxels") - def test_contrast_detection( + def test_contrast_group_is_absent( self, segmenter_simpleware: SegmentHeartSimpleware, test_images: list[Any], ) -> None: - """Test contrast mask is returned (base class behavior).""" + """ASCardio does not detect contrast, so no contrast group is reported. + + SegmentAnatomyBase used to seed a contrast placeholder that this + segmenter inherited; it seeds nothing now, so callers must check for + the key instead of indexing it. + """ if not _simpleware_available(segmenter_simpleware): pytest.skip("Simpleware Medical not found. Install to run this test.") input_image = test_images[3] result = segmenter_simpleware.segment(input_image) - contrast_mask = result["contrast"] - assert contrast_mask is not None - assert itk.size(contrast_mask) == itk.size(input_image) - print("\nContrast mask returned") + assert "contrast" not in result def test_postprocessing( self, diff --git a/tests/test_workflow_convert_image_to_usd.py b/tests/test_workflow_convert_image_to_usd.py index 78f7c83..33dada3 100644 --- a/tests/test_workflow_convert_image_to_usd.py +++ b/tests/test_workflow_convert_image_to_usd.py @@ -124,7 +124,7 @@ def test_workflow_convert_image_to_usd_default_operation( result_filenames = workflow.process() assert result_filenames == {"all": "slicer_heart_small.all_painted.usd"} - assert workflow.reference_segmentation is not None + assert workflow.reference_segmentation_results is not None assert "all" in workflow.reference_contours assert len(workflow.transformed_contours["all"]) == 1 assert len(workflow.registration_results) == 1 @@ -134,13 +134,15 @@ def test_workflow_convert_image_to_usd_default_operation( reference_labelmap = cast( itk.Image, - workflow.reference_segmentation["labelmap"], + workflow.reference_segmentation_results["labelmap"], ) assert itk.size(reference_labelmap) == itk.size(reference_image) + # No slice_NNN_labelmap.mha here: the workflow segments each moving frame + # only when dynamic_labelmap_ids is non-empty, and the default operation + # this test covers leaves it empty. expected_outputs = [ "reference_labelmap.mha", - "slice_000_labelmap.mha", "slicer_heart_small.all.usd", "slicer_heart_small.all_painted.usd", ] diff --git a/tests/test_workflow_fit_statistical_model_to_patient.py b/tests/test_workflow_fit_statistical_model_to_patient.py index 633fcc9..ca1bc13 100644 --- a/tests/test_workflow_fit_statistical_model_to_patient.py +++ b/tests/test_workflow_fit_statistical_model_to_patient.py @@ -200,3 +200,65 @@ def test_fit_icp_transform_type_defaults_to_affine_and_validates() -> None: with pytest.raises(ValueError, match="Invalid ICP transform"): workflow.set_icp_transform_type("Deformable") + + +def _fit_workflow_for_pca() -> WorkflowFitStatisticalModelToPatient: + """A minimal fit workflow, for exercising the PCA configuration only.""" + image = itk.image_from_array(np.zeros((3, 3, 3), dtype=np.float32)) + model = pv.PolyData(np.zeros((3, 3), dtype=np.float64)) + return WorkflowFitStatisticalModelToPatient( + template_model=model, + patient_models=[model], + patient_image=image, + ) + + +def test_requested_pca_components_drop_to_what_the_model_carries() -> None: + """A model built from a small population carries fewer modes than asked for. + + WorkflowCreateStatisticalModel caps a model at one fewer mode than it had + samples, so a count configured for a full population is too large for a + model built from a handful of cases. Asking the optimizer for modes that + do not exist raises partway through the fit, so the count is reduced here. + """ + workflow = _fit_workflow_for_pca() + pca_model = {"eigenvalues": [4.0, 1.0], "components": [[0.0], [0.0]]} + + workflow.set_use_pca_registration( + use_pca_registration=True, + pca_model=pca_model, + number_of_pca_components=5, + ) + + assert workflow.number_of_pca_components == 2 + + +def test_requested_pca_components_are_kept_when_the_model_carries_them() -> None: + """Reducing the count must not touch a request the model can satisfy.""" + workflow = _fit_workflow_for_pca() + pca_model = { + "eigenvalues": [4.0, 2.0, 1.0], + "components": [[0.0], [0.0], [0.0]], + } + + workflow.set_use_pca_registration( + use_pca_registration=True, + pca_model=pca_model, + number_of_pca_components=2, + ) + + assert workflow.number_of_pca_components == 2 + + +def test_pca_component_count_of_zero_still_means_every_mode() -> None: + """0 is the documented "use all" sentinel and must survive the reduction.""" + workflow = _fit_workflow_for_pca() + pca_model = {"eigenvalues": [4.0, 1.0], "components": [[0.0], [0.0]]} + + workflow.set_use_pca_registration( + use_pca_registration=True, + pca_model=pca_model, + number_of_pca_components=0, + ) + + assert workflow.number_of_pca_components == 0 diff --git a/tutorials/parameters_lung_ct_dirlab.py b/tutorials/parameters_lung_ct_dirlab.py index 06e0da7..0b29e13 100644 --- a/tutorials/parameters_lung_ct_dirlab.py +++ b/tutorials/parameters_lung_ct_dirlab.py @@ -45,6 +45,9 @@ class ParametersLungCTDirLab(ParametersBase): mesh_reduction_rate: Fraction of the voxel resolution removed along each axis before a labelmap is meshed into tetrahedra, so the tetrahedron count falls by roughly ``(1 - rate) ** 3``. + model_points: Points kept per surface when building the shape model. + ``0`` keeps every point, which is what a full run does. + model_points_test: Same, under ``TestTools.running_as_test``. number_of_pca_components: PCA components retained when building the lung statistical model, and used when fitting it to a patient. number_of_pca_components_test: Same, under ``TestTools.running_as_test``. @@ -83,6 +86,9 @@ class ParametersLungCTDirLab(ParametersBase): surface_reduction_rate: float = 0.0 mesh_reduction_rate: float = 0.0 + model_points: int = 0 + model_points_test: int = 20000 + number_of_pca_components: int = 6 number_of_pca_components_test: int = 5 @@ -135,6 +141,10 @@ def pca_components(self, test_mode: bool) -> int: else (self.number_of_pca_components) ) + def points_per_model(self, test_mode: bool) -> int: + """Return the per-surface point budget for this run mode.""" + return self.model_points_test if test_mode else self.model_points + def greedy_iterations(self, test_mode: bool) -> list[int]: """Return the Greedy iteration schedule for this run mode.""" return list( diff --git a/tutorials/tutorial_06_lung_create_statistical_model.py b/tutorials/tutorial_06_lung_create_statistical_model.py index 8fea7c6..5e09b27 100644 --- a/tutorials/tutorial_06_lung_create_statistical_model.py +++ b/tutorials/tutorial_06_lung_create_statistical_model.py @@ -35,7 +35,7 @@ import json import logging from pathlib import Path -from typing import Any +from typing import Any, cast import itk import numpy as np @@ -79,6 +79,11 @@ # template-biased pass. mean_surface_iterations = 1 if test_mode else 3 + # Points kept per surface; 0 keeps every point. The lung surfaces feed + # both the atlas below and the model after it, so reducing them here cuts + # the cost of each. + model_points = LUNG_CT_DIRLAB.points_per_model(test_mode) + # Distance-map weights finetuned by # tutorial_02_lung_distancemap_finetune_icon.py. Stock uniGradICON weights # are out of distribution for distance maps, so without these the @@ -133,7 +138,12 @@ output_dir / f"{sample_image_file.stem}_labelmap.nii.gz" ) itk.imwrite(sample_labelmap, str(sample_labelmap_file), compression=True) - sample_surfaces.append(pv.read(str(sample_surface_file))) + sample_surface = cast(pv.PolyData, pv.read(str(sample_surface_file))) + if model_points: + sample_surface = contour_tools.remesh_and_smooth_surface( + sample_surface, 1.0 - model_points / sample_surface.n_points, 0 + ) + sample_surfaces.append(sample_surface) # The reference surface defines the topology every PCA input is expressed # in, so picking one case makes the model inherit that case's shape. Use the @@ -146,6 +156,7 @@ # another is the one way the two can disagree without saying so. mean_surface_settings = { "iterations": mean_surface_iterations, + "model_points": model_points, "mask_dilation_mm": LUNG_CT_DIRLAB.mask_dilation_mm, "distance_squared_max": LUNG_CT_DIRLAB.distancemap_squared_max, "icon_weights": ( @@ -184,6 +195,10 @@ sample_meshes=sample_surfaces, reference_mesh=reference_surface, number_of_pca_components=number_of_pca_components, + # The distance maps step 3 registers are rasterized at this resolution, + # and generating, dilating and affinely registering them is what the + # step costs. 2 mm is an eighth of the voxels of the 1 mm default. + reference_spatial_resolution=2.0 if test_mode else 1.0, icp_transform_type=LUNG_CT_DIRLAB.icp_transform_type, mask_dilation_mm=LUNG_CT_DIRLAB.mask_dilation_mm, distance_squared_max=LUNG_CT_DIRLAB.distancemap_squared_max, From 582a791cd1200c70517ce81a2d33d237b8ff5c8c Mon Sep 17 00:00:00 2001 From: Stephen Aylward Date: Thu, 27 Aug 2026 12:03:46 -0400 Subject: [PATCH 2/2] AI: Coderabbit --- .../segment_chest_total_segmentator.py | 17 ++++++- ...rkflow_fit_statistical_model_to_patient.py | 2 +- tests/test_segment_chest_total_segmentator.py | 37 ++++++++++++++ tests/test_segment_heart_simpleware.py | 15 +++--- ...rkflow_fit_statistical_model_to_patient.py | 51 +++++++++++++++++++ 5 files changed, 112 insertions(+), 10 deletions(-) diff --git a/src/physiotwin4d/segment_chest_total_segmentator.py b/src/physiotwin4d/segment_chest_total_segmentator.py index 9dbf224..122a814 100644 --- a/src/physiotwin4d/segment_chest_total_segmentator.py +++ b/src/physiotwin4d/segment_chest_total_segmentator.py @@ -227,7 +227,22 @@ def __init__(self, log_level: int | str = logging.INFO): @staticmethod def _academic_license_is_valid() -> bool: - """Return True when TotalSegmentator reports an installed license.""" + """Return True when TotalSegmentator reports an installed license. + + Deliberately the same offline check ``show_license_info`` performs + before a licensed task, so this predicts exactly whether that call + would exit. The offline check only tests that a license number is + configured and 18 characters long, so a stale or revoked key of the + right length still reads as installed; TotalSegmentator would then + exit while downloading the licensed weights, which no pre-check of + ours can prevent. + + ``has_valid_license`` would catch that by asking the backend, but it + reports a network failure as ``invalid_license`` too, so a runner that + is merely offline would silently segment without the licensed tasks + and quietly produce different anatomy. Wrongly degrading a valid + licensed run is worse than the revoked-key case this misses. + """ from totalsegmentator.libs import ( # noqa: PLC0415 has_valid_license_offline, ) diff --git a/src/physiotwin4d/workflow_fit_statistical_model_to_patient.py b/src/physiotwin4d/workflow_fit_statistical_model_to_patient.py index f454c26..b299b38 100644 --- a/src/physiotwin4d/workflow_fit_statistical_model_to_patient.py +++ b/src/physiotwin4d/workflow_fit_statistical_model_to_patient.py @@ -409,7 +409,7 @@ def set_use_pca_registration( # exist raises out of the optimizer partway through the fit. Read # the count the model actually carries instead. available_components = len(pca_model.get("components", [])) - if 0 < available_components < number_of_pca_components: + if available_components < number_of_pca_components: self.log_info( "PCA model carries %d mode(s), fewer than the %d requested; " "fitting with the %d available.", diff --git a/tests/test_segment_chest_total_segmentator.py b/tests/test_segment_chest_total_segmentator.py index e27778f..ce807b9 100644 --- a/tests/test_segment_chest_total_segmentator.py +++ b/tests/test_segment_chest_total_segmentator.py @@ -330,3 +330,40 @@ def _fail() -> bool: segmenter.set_has_academic_license(False) assert segmenter.has_academic_license is False + + +def test_license_check_tracks_totalsegmentators_own_offline_gate( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The check must agree with the gate it is predicting, weakness included. + + ``show_license_info`` exits unless ``has_valid_license_offline`` returns + "yes", and that only tests for a configured 18-character key. A key of the + right length but no longer entitled therefore reads as installed here, and + TotalSegmentator fails later, while downloading the licensed weights. + Asking the backend instead would report an offline runner as unlicensed and + silently change the anatomy it produces, so the weaker check is the + deliberate choice and this pins it. + """ + import totalsegmentator.libs as ts_libs + + monkeypatch.setattr( + ts_libs, + "has_valid_license_offline", + lambda: ("yes", "SUCCESS: License is valid."), + ) + assert SegmentChestTotalSegmentator._academic_license_is_valid() is True + + monkeypatch.setattr( + ts_libs, + "has_valid_license_offline", + lambda: ("invalid_license", "ERROR: Invalid license number (too-short)."), + ) + assert SegmentChestTotalSegmentator._academic_license_is_valid() is False + + monkeypatch.setattr( + ts_libs, + "has_valid_license_offline", + lambda: ("missing_license", "ERROR: A license number has not been set."), + ) + assert SegmentChestTotalSegmentator._academic_license_is_valid() is False diff --git a/tests/test_segment_heart_simpleware.py b/tests/test_segment_heart_simpleware.py index 19740c1..e2b172c 100644 --- a/tests/test_segment_heart_simpleware.py +++ b/tests/test_segment_heart_simpleware.py @@ -176,14 +176,13 @@ def test_anatomy_group_labelmaps( unique_values = set(np.unique(group_labelmap_arr).tolist()) assert 0 in unique_values, f"{group} labelmap should contain background" - # "other" collects whatever ids no group claimed, so it has no - # fixed id set to check against. - if group != "other": - allowed_values = {0} | set(taxonomy.labels_in_group(group).keys()) - assert unique_values <= allowed_values, ( - f"{group} labelmap contains unexpected label ids: " - f"{unique_values - allowed_values}" - ) + # "other" is checked too: _finalize_other_group claims every id + # no other group took, so it has just as fixed an id set. + allowed_values = {0} | set(taxonomy.labels_in_group(group).keys()) + assert unique_values <= allowed_values, ( + f"{group} labelmap contains unexpected label ids: " + f"{unique_values - allowed_values}" + ) assert itk.size(group_labelmap) == itk.size(input_image), ( f"{group} labelmap size mismatch" diff --git a/tests/test_workflow_fit_statistical_model_to_patient.py b/tests/test_workflow_fit_statistical_model_to_patient.py index ca1bc13..4b0c23e 100644 --- a/tests/test_workflow_fit_statistical_model_to_patient.py +++ b/tests/test_workflow_fit_statistical_model_to_patient.py @@ -262,3 +262,54 @@ def test_pca_component_count_of_zero_still_means_every_mode() -> None: ) assert workflow.number_of_pca_components == 0 + + +def test_empty_pca_model_reduces_a_positive_request_to_zero() -> None: + """A model carrying no modes must not leave a positive request standing. + + ``RegisterModelsPCA`` raises when asked for more modes than it holds, so a + request that survived an empty model would fail inside the optimizer rather + than here. Zero is the documented "use every mode" sentinel, which for an + empty model is zero modes. + """ + workflow = _fit_workflow_for_pca() + + workflow.set_use_pca_registration( + use_pca_registration=True, + pca_model={"eigenvalues": [], "components": []}, + number_of_pca_components=5, + ) + + assert workflow.number_of_pca_components == 0 + + +def test_clamped_component_count_reaches_the_pca_registrar() -> None: + """The reduced count must be what register_model_to_model_pca passes on. + + Reducing the stored count would be pointless if the registrar were built + from the originally requested one. + """ + captured: dict[str, Any] = {} + + def _capture(**kwargs: Any) -> None: + captured.update(kwargs) + raise RuntimeError("stop after capturing the registrar configuration") + + workflow = _fit_workflow_for_pca() + workflow.set_use_pca_registration( + use_pca_registration=True, + pca_model={"eigenvalues": [4.0, 1.0], "components": [[0.0], [0.0]]}, + number_of_pca_components=5, + ) + + from physiotwin4d import workflow_fit_statistical_model_to_patient as module + + original = module.RegisterModelsPCA.from_pca_model + module.RegisterModelsPCA.from_pca_model = staticmethod(_capture) + try: + with pytest.raises(RuntimeError): + workflow.register_model_to_model_pca() + finally: + module.RegisterModelsPCA.from_pca_model = original + + assert captured["pca_number_of_modes"] == 2