From e7627eb6896a40c239b2096b6a2d1504dc5852b9 Mon Sep 17 00:00:00 2001 From: "Austin E. Y. T. Lefebvre" Date: Tue, 12 May 2026 13:56:31 -0700 Subject: [PATCH] napari/settings: replace single checkbox with per-output retention group + presets (Slice 3 of #245) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the legacy "Remove intermediate files" checkbox with a new ``IntermediateRetentionGroup`` widget composed of: - A preset radio row at top (Keep everything / Masks + CSVs only / CSVs only / Custom). Custom is auto-selected when the per-output selection diverges from any named preset; not user-clickable. - Eight stage-grouped subgroups (Filter, Label, Network, Markers, Tracking, Voxel reassignment, Hierarchy, Pipeline input) holding one checkbox per key in ``DROPPABLE_KEYS``. Checkbox semantics: **checked = drop**. Each checkbox carries a tooltip describing the source stage and downstream consumers (re-run implications). Selecting a named preset radio sets the per-output checkboxes to match the preset's drop set. Toggling any individual checkbox flips the radio to "Custom"; toggling back to the preset's exact drop set re-selects the preset. ``SettingsConfig`` schema change: ``remove_intermediates: bool`` → ``cleanup_drop_keys: frozenset[str]``. ``to_config`` reads the widget's ``drop_keys()``; ``apply_config`` calls ``set_drop_keys(...)`` which also syncs the preset radio. 17 new tests in ``tests/test_napari_settings.py`` pin: default state ("Keep everything", empty drop set); checkbox count and membership; tooltips present; preset → drop-set mapping (×3); ``set_drop_keys`` round-trip (×3); preset radio reflects loaded drop set (named + Custom); checkbox toggle after preset switches to Custom (and back); unknown / CSV keys rejected; ``SettingsConfig`` field schema and round-trip. Note: ``nellie_processor.py`` still references the now-removed ``remove_intermediates_checkbox``; that attribute lookup will fail at runtime in ``run_feature_export`` until Slice 4 (#249) re-wires it. No tests exercise that path so the regression suite stays green (715 passed). Closes #248 Co-Authored-By: Claude Opus 4.7 (1M context) --- nellie_napari/nellie_settings.py | 209 +++++++++++++++++++++++++++++-- tests/test_napari_settings.py | 195 ++++++++++++++++++++++++++++ 2 files changed, 393 insertions(+), 11 deletions(-) create mode 100644 tests/test_napari_settings.py diff --git a/nellie_napari/nellie_settings.py b/nellie_napari/nellie_settings.py index cec6ee6..0691ada 100644 --- a/nellie_napari/nellie_settings.py +++ b/nellie_napari/nellie_settings.py @@ -14,9 +14,17 @@ QDoubleSpinBox, QComboBox, QScrollArea, + QButtonGroup, + QRadioButton, ) from nellie.feature_extraction.hierarchical import HierarchyConfig +from nellie.im_info.verifier import ( + CSVS_ONLY_PRESET, + DROPPABLE_KEYS, + KEEP_EVERYTHING_PRESET, + MASKS_AND_CSVS_PRESET, +) from nellie.segmentation.filtering import FrangiConfig from nellie.segmentation.labelling import LabelConfig from nellie.segmentation.mocap_marking import MarkersConfig @@ -25,13 +33,197 @@ from nellie.tracking.voxel_reassignment import VoxelReassignerConfig +# Per-key presentation metadata for the IntermediateRetentionGroup widget. +# Stage groupings, friendly labels, and tooltips. Order within each stage +# controls UI display order. See [[wiki/decisions/0014-intermediates-policy-frozenset]]. +_RETENTION_STAGE_GROUPS: list[tuple[str, list[tuple[str, str, str]]]] = [ + ('Filter', [ + ('im_preprocessed', 'Frangi vesselness', + 'Generated by: Filter (Frangi vesselness). Required to re-run Label.'), + ]), + ('Label', [ + ('im_instance_label', 'Instance segmentation labels', + 'Generated by: Label. Required by Network, Markers, Hu, Voxel reassignment, Hierarchy.'), + ]), + ('Network', [ + ('im_skel', 'Skeleton', + 'Generated by: Network. Required by Markers and Hierarchy.'), + ('im_skel_relabelled', 'Branch-relabelled skeleton', + 'Generated by: Network. Required by Hierarchy.'), + ('im_pixel_class', 'Pixel classification', + 'Generated by: Network. Required by Hierarchy.'), + ]), + ('Markers', [ + ('im_marker', 'Motion-anchor markers', + 'Generated by: Markers. Required by Hu tracking.'), + ('im_distance', 'Distance transform', + 'Generated by: Markers. Required by Hierarchy.'), + ('im_border', 'Border map', + 'Generated by: Markers (internal).'), + ]), + ('Tracking', [ + ('flow_vector_array', 'Flow vectors', + 'Generated by: Hu tracking. Required by Voxel reassignment and Hierarchy.'), + ('voxel_matches', 'Voxel matches', + 'Generated by: Hu tracking. Required by Voxel reassignment.'), + ]), + ('Voxel reassignment', [ + ('im_branch_label_reassigned', 'Tracked branch labels', + 'Generated by: Voxel reassignment. Required by Hierarchy.'), + ('im_obj_label_reassigned', 'Tracked object labels', + 'Generated by: Voxel reassignment. Required by Hierarchy.'), + ]), + ('Hierarchy', [ + ('adjacency_maps', 'Node-edge adjacency maps', + 'Generated by: Hierarchy (only when skip_nodes=False). ' + 'Read by the napari analyzer post-pipeline.'), + ]), + ('Pipeline input', [ + ('im_path', 'Canonical OME-TIFF (regenerable from source)', + 'Generated by: ImInfo on first load. Read by every pipeline stage. ' + 'Regenerable from the original microscopy file (expensive).'), + ]), +] + + +_PRESET_NAME_TO_DROP_SET: dict[str, frozenset[str]] = { + 'Keep everything': KEEP_EVERYTHING_PRESET, + 'Masks + CSVs only': MASKS_AND_CSVS_PRESET, + 'CSVs only': CSVS_ONLY_PRESET, +} +_PRESET_DROP_SET_TO_NAME: dict[frozenset[str], str] = { + v: k for k, v in _PRESET_NAME_TO_DROP_SET.items() +} +_CUSTOM_PRESET_NAME = 'Custom' + + +class IntermediateRetentionGroup(QGroupBox): + """Per-output intermediate retention controls. + + Composition: a preset radio row at top + stage-grouped checkboxes + (one per key in :data:`DROPPABLE_KEYS`). Checkbox semantics: + **checked = drop**, unchecked = keep. Selecting a named preset + (Keep everything / Masks + CSVs only / CSVs only) sets the + checkboxes; toggling any checkbox flips the preset radio to + "Custom" automatically. + + The "Custom" radio is set programmatically and is not user-clickable; + it is purely informational ("your selection diverges from any named + preset"). + + Read the current drop set via :meth:`drop_keys`; set it via + :meth:`set_drop_keys`. + """ + + def __init__(self, parent: Optional[QWidget] = None) -> None: + super().__init__("Intermediate file retention", parent) + + self._key_checkboxes: dict[str, QCheckBox] = {} + self._preset_buttons: dict[str, QRadioButton] = {} + self._suppress_signals = False + + outer = QVBoxLayout() + + # Preset radio row. + preset_row = QHBoxLayout() + preset_row.addWidget(QLabel("Preset:")) + self._preset_button_group = QButtonGroup(self) + for name in (*_PRESET_NAME_TO_DROP_SET.keys(), _CUSTOM_PRESET_NAME): + btn = QRadioButton(name) + self._preset_button_group.addButton(btn) + self._preset_buttons[name] = btn + preset_row.addWidget(btn) + # Default selection. + self._preset_buttons['Keep everything'].setChecked(True) + # "Custom" is informational; user selects it indirectly via toggling + # any individual checkbox. Disable direct clicks but keep it visible. + self._preset_buttons[_CUSTOM_PRESET_NAME].setEnabled(False) + # Wire user-clickable presets to apply the matching drop set. + for name in _PRESET_NAME_TO_DROP_SET: + self._preset_buttons[name].toggled.connect( + lambda checked, n=name: self._on_preset_clicked(n, checked) + ) + outer.addLayout(preset_row) + + # Per-stage checkbox subgroups. + for stage_name, entries in _RETENTION_STAGE_GROUPS: + stage_box = QGroupBox(stage_name) + stage_layout = QVBoxLayout() + for key, friendly, tooltip in entries: + cb = QCheckBox(f"{friendly} (`{key}`)") + cb.setToolTip(tooltip) + cb.toggled.connect(self._on_checkbox_toggled) + self._key_checkboxes[key] = cb + stage_layout.addWidget(cb) + stage_box.setLayout(stage_layout) + outer.addWidget(stage_box) + + self.setLayout(outer) + + # ------------------------------------------------------------------ + # Public API + # ------------------------------------------------------------------ + + def drop_keys(self) -> frozenset[str]: + """Return the current drop set (keys whose checkbox is checked).""" + return frozenset( + key for key, cb in self._key_checkboxes.items() if cb.isChecked() + ) + + def set_drop_keys(self, drop_keys: frozenset[str]) -> None: + """Apply a drop set to the checkboxes; sync the preset radio. + + Validates that ``drop_keys`` is a subset of ``DROPPABLE_KEYS``. + Selects the matching named preset radio (or "Custom" if no + named preset matches). + """ + unknown = set(drop_keys) - DROPPABLE_KEYS + assert not unknown, f"set_drop_keys: unknown keys {sorted(unknown)}" + self._suppress_signals = True + try: + for key, cb in self._key_checkboxes.items(): + cb.setChecked(key in drop_keys) + self._sync_preset_radio_to_drop_set(drop_keys) + finally: + self._suppress_signals = False + + # ------------------------------------------------------------------ + # Internals + # ------------------------------------------------------------------ + + def _on_preset_clicked(self, name: str, checked: bool) -> None: + if not checked or self._suppress_signals: + return + target = _PRESET_NAME_TO_DROP_SET[name] + self._suppress_signals = True + try: + for key, cb in self._key_checkboxes.items(): + cb.setChecked(key in target) + finally: + self._suppress_signals = False + + def _on_checkbox_toggled(self, _checked: bool) -> None: + if self._suppress_signals: + return + self._sync_preset_radio_to_drop_set(self.drop_keys()) + + def _sync_preset_radio_to_drop_set(self, drop_keys: frozenset[str]) -> None: + name = _PRESET_DROP_SET_TO_NAME.get(drop_keys, _CUSTOM_PRESET_NAME) + # Temporarily allow Custom to be programmatically selected. + was_enabled = self._preset_buttons[_CUSTOM_PRESET_NAME].isEnabled() + self._preset_buttons[_CUSTOM_PRESET_NAME].setEnabled(True) + # exclusive=True (default) ensures only one is checked. + self._preset_buttons[name].setChecked(True) + self._preset_buttons[_CUSTOM_PRESET_NAME].setEnabled(was_enabled) + + @dataclass class SettingsConfig: """ Structured representation of the Settings UI state. """ remove_edges: bool - remove_intermediates: bool + cleanup_drop_keys: frozenset[str] voxel_reassign: bool analyze_node_level: bool track_all_frames: bool @@ -140,13 +332,7 @@ def __init__(self, napari_viewer: "napari.viewer.Viewer", nellie, parent=None): "weird image edge artifacts." ) - self.remove_intermediates_checkbox = QCheckBox("Remove intermediate files") - self.remove_intermediates_checkbox.setChecked(False) - self.remove_intermediates_checkbox.setEnabled(True) - self.remove_intermediates_checkbox.setToolTip( - "Remove intermediate files after processing. This means only CSV files " - "will be saved and intermediate data cannot be recovered." - ) + self.intermediate_retention_group = IntermediateRetentionGroup() self.voxel_reassign = QCheckBox("Auto-run voxel reassignment") self.voxel_reassign.setChecked(True) @@ -435,8 +621,8 @@ def set_ui(self): processor_layout = QVBoxLayout() subprocessor_layout1 = QHBoxLayout() - subprocessor_layout1.addWidget(self.remove_intermediates_checkbox) subprocessor_layout1.addWidget(self.remove_edges_checkbox) + subprocessor_layout1.addStretch(1) subprocessor_layout2 = QHBoxLayout() subprocessor_layout2.addWidget(self.analyze_node_level) @@ -444,6 +630,7 @@ def set_ui(self): processor_layout.addLayout(subprocessor_layout1) processor_layout.addLayout(subprocessor_layout2) + processor_layout.addWidget(self.intermediate_retention_group) processor_group.setLayout(processor_layout) # Tracking settings group @@ -642,7 +829,7 @@ def to_config(self) -> SettingsConfig: """ return SettingsConfig( remove_edges=self.remove_edges_checkbox.isChecked(), - remove_intermediates=self.remove_intermediates_checkbox.isChecked(), + cleanup_drop_keys=self.intermediate_retention_group.drop_keys(), voxel_reassign=self.voxel_reassign.isChecked(), analyze_node_level=self.analyze_node_level.isChecked(), track_all_frames=self.track_all_frames.isChecked(), @@ -743,7 +930,7 @@ def apply_config(self, config: SettingsConfig): Configuration to apply to the UI. """ self.remove_edges_checkbox.setChecked(config.remove_edges) - self.remove_intermediates_checkbox.setChecked(config.remove_intermediates) + self.intermediate_retention_group.set_drop_keys(config.cleanup_drop_keys) self.voxel_reassign.setChecked(config.voxel_reassign) self.analyze_node_level.setChecked(config.analyze_node_level) self.track_all_frames.setChecked(config.track_all_frames) diff --git a/tests/test_napari_settings.py b/tests/test_napari_settings.py new file mode 100644 index 0000000..a1b91dd --- /dev/null +++ b/tests/test_napari_settings.py @@ -0,0 +1,195 @@ +"""Tests for the napari Settings widget's intermediate-retention surface. + +Slice 3 of #245 replaces the legacy single ``remove_intermediates_checkbox`` +with a per-output ``IntermediateRetentionGroup`` and switches the +``SettingsConfig`` schema field from ``remove_intermediates: bool`` to +``cleanup_drop_keys: frozenset[str]``. + +These tests pin the contract via the public ``IntermediateRetentionGroup`` +API (``drop_keys()`` / ``set_drop_keys(...)``) and the +``SettingsConfig`` round-trip. We instantiate the widget directly (no +napari viewer needed) to keep tests fast and isolated. Each test +constructs its own widget instance — the widget is cheap to create +once a single ``QApplication`` exists for the test session. +""" + +from __future__ import annotations + +import pytest + +from nellie.im_info.verifier import ( + CSVS_ONLY_PRESET, + DROPPABLE_KEYS, + KEEP_EVERYTHING_PRESET, + MASKS_AND_CSVS_PRESET, +) + + +@pytest.fixture(scope="session") +def qapp(): + """Provide a QApplication for the test session. + + pytest-qt is not a project dependency; this fixture is the minimum + needed to instantiate QWidgets in isolation. + """ + from qtpy.QtWidgets import QApplication + app = QApplication.instance() or QApplication([]) + yield app + # Don't quit — other tests in the session may need the app. + + +@pytest.fixture +def retention_group(qapp): + from nellie_napari.nellie_settings import IntermediateRetentionGroup + return IntermediateRetentionGroup() + + +# --------------------------------------------------------------------------- +# IntermediateRetentionGroup +# --------------------------------------------------------------------------- + + +def test_default_drop_keys_is_empty_frozenset(retention_group) -> None: + """Fresh widget defaults to "Keep everything" (empty drop set).""" + assert retention_group.drop_keys() == frozenset() + + +def test_default_preset_radio_is_keep_everything(retention_group) -> None: + """The "Keep everything" preset radio is the default selection.""" + assert retention_group._preset_buttons['Keep everything'].isChecked() + assert not retention_group._preset_buttons['Custom'].isChecked() + + +def test_widget_has_one_checkbox_per_droppable_key(retention_group) -> None: + """Every key in DROPPABLE_KEYS has a checkbox; no extras, no missing.""" + assert set(retention_group._key_checkboxes.keys()) == DROPPABLE_KEYS + + +def test_widget_checkboxes_carry_tooltips(retention_group) -> None: + """Each checkbox tooltip is non-empty (PRD User Story 10).""" + for key, cb in retention_group._key_checkboxes.items(): + assert cb.toolTip(), f"checkbox for {key!r} has no tooltip" + + +@pytest.mark.parametrize('preset_name,preset_set', [ + ('Keep everything', KEEP_EVERYTHING_PRESET), + ('Masks + CSVs only', MASKS_AND_CSVS_PRESET), + ('CSVs only', CSVS_ONLY_PRESET), +]) +def test_clicking_preset_radio_sets_drop_keys( + retention_group, preset_name: str, preset_set: frozenset[str], +) -> None: + """Clicking each named preset snaps the drop set to the preset's value.""" + retention_group._preset_buttons[preset_name].setChecked(True) + assert retention_group.drop_keys() == preset_set + + +@pytest.mark.parametrize('preset_set', [ + KEEP_EVERYTHING_PRESET, + MASKS_AND_CSVS_PRESET, + CSVS_ONLY_PRESET, +]) +def test_set_drop_keys_round_trip_preserves_preset( + retention_group, preset_set: frozenset[str], +) -> None: + """``set_drop_keys`` then ``drop_keys`` round-trips the preset.""" + retention_group.set_drop_keys(preset_set) + assert retention_group.drop_keys() == preset_set + + +def test_set_drop_keys_with_named_preset_selects_matching_radio( + retention_group, +) -> None: + """Loading a named preset's drop set selects the corresponding radio.""" + retention_group.set_drop_keys(MASKS_AND_CSVS_PRESET) + assert retention_group._preset_buttons['Masks + CSVs only'].isChecked() + assert not retention_group._preset_buttons['Custom'].isChecked() + + +def test_set_drop_keys_with_non_preset_set_selects_custom_radio( + retention_group, +) -> None: + """A drop set that doesn't match any named preset selects "Custom".""" + retention_group.set_drop_keys(frozenset({'im_preprocessed'})) + assert retention_group._preset_buttons['Custom'].isChecked() + for name in ('Keep everything', 'Masks + CSVs only', 'CSVs only'): + assert not retention_group._preset_buttons[name].isChecked(), ( + f"named preset {name!r} should not be selected for a custom set" + ) + + +def test_toggling_checkbox_after_preset_switches_to_custom( + retention_group, +) -> None: + """Toggling any checkbox after a preset selection flips to "Custom".""" + retention_group.set_drop_keys(KEEP_EVERYTHING_PRESET) + assert retention_group._preset_buttons['Keep everything'].isChecked() + + retention_group._key_checkboxes['im_preprocessed'].setChecked(True) + assert retention_group._preset_buttons['Custom'].isChecked() + assert not retention_group._preset_buttons['Keep everything'].isChecked() + + +def test_toggling_checkbox_back_to_preset_state_re_selects_preset( + retention_group, +) -> None: + """Toggling all the way back to a preset's drop set re-selects that preset.""" + retention_group.set_drop_keys(KEEP_EVERYTHING_PRESET) + cb = retention_group._key_checkboxes['im_preprocessed'] + cb.setChecked(True) + assert retention_group._preset_buttons['Custom'].isChecked() + cb.setChecked(False) + assert retention_group._preset_buttons['Keep everything'].isChecked() + + +def test_set_drop_keys_rejects_unknown_keys(retention_group) -> None: + """Unknown keys raise AssertionError (CSV keys also rejected).""" + with pytest.raises(AssertionError): + retention_group.set_drop_keys(frozenset({'not_a_real_key'})) + with pytest.raises(AssertionError): + retention_group.set_drop_keys(frozenset({'features_voxels'})) + + +# --------------------------------------------------------------------------- +# SettingsConfig schema (no widget needed) +# --------------------------------------------------------------------------- + + +def test_settings_config_has_cleanup_drop_keys_field() -> None: + """``SettingsConfig`` exposes ``cleanup_drop_keys: frozenset[str]``.""" + from dataclasses import fields + from nellie_napari.nellie_settings import SettingsConfig + + field_names = {f.name for f in fields(SettingsConfig)} + assert 'cleanup_drop_keys' in field_names + # Legacy field is gone. + assert 'remove_intermediates' not in field_names + + +def test_settings_config_cleanup_drop_keys_accepts_frozenset() -> None: + """Construction with a frozenset[str] cleanup_drop_keys works.""" + # Build a minimal config; only assert the cleanup_drop_keys field + # round-trips (other fields are unrelated to this slice). + from dataclasses import fields + from nellie_napari.nellie_settings import SettingsConfig + + # All field names except cleanup_drop_keys (we'll set those to dummies). + other_kwargs = {} + for f in fields(SettingsConfig): + if f.name == 'cleanup_drop_keys': + continue + # Best-effort dummy by type annotation. + ann = f.type + if ann == 'bool' or ann is bool: + other_kwargs[f.name] = False + elif ann == 'int' or ann is int: + other_kwargs[f.name] = 0 + elif ann == 'float' or ann is float: + other_kwargs[f.name] = 0.0 + elif ann == 'str' or ann is str: + other_kwargs[f.name] = '' + else: + other_kwargs[f.name] = None + cfg = SettingsConfig(cleanup_drop_keys=CSVS_ONLY_PRESET, **other_kwargs) + assert cfg.cleanup_drop_keys == CSVS_ONLY_PRESET + assert isinstance(cfg.cleanup_drop_keys, frozenset)