From 82afc226e7c28ec14726f7c9a72c6e7d7814aca4 Mon Sep 17 00:00:00 2001 From: Hemanth Kapa Date: Thu, 2 Jul 2026 13:04:49 -0700 Subject: [PATCH 1/7] Allow accepting a different refinement iteration per component IMM and OMM converge differently, so a single global "iteration to accept" forced the same choice on both. Replace the single spinbox with one step spinbox per component, discovered from the *_refined_iter{N} surfaces on disk, and dispatch one `accept_refinement --component X ` call per component. accept_refinement's cleanup is scoped to each basename, so accepting one component never deletes another's iterations. - Per-component rows rebuilt on config load, after a run, and via a new Refresh button; default to the final (converged) iteration, range spans available iterations, prior selections preserved across refreshes. - Accept validates each choice exists, confirms all choices, and reports per-component failures; disabled when no refined surfaces exist. - Tests for discovery/ranges/defaults, selection preservation, empty-state disable, and one --component CLI call per component. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../jobs/refinement_tab.py | 146 +++++++++++++----- tests/test_refinement_tab.py | 69 ++++++++- 2 files changed, 178 insertions(+), 37 deletions(-) diff --git a/src/surface_morphometrics_gui/jobs/refinement_tab.py b/src/surface_morphometrics_gui/jobs/refinement_tab.py index dfccb3e..ff1bfd7 100644 --- a/src/surface_morphometrics_gui/jobs/refinement_tab.py +++ b/src/surface_morphometrics_gui/jobs/refinement_tab.py @@ -1,5 +1,6 @@ import copy import os +import re import subprocess import threading from pathlib import Path @@ -145,16 +146,20 @@ def __init__(self, experiment_manager): # --- Accept an iteration (destructive: promotes one, removes the rest) --- inner_layout.addWidget(QLabel("Accept Iteration")) inner_layout.addWidget(QLabel( - "Promote one iteration to be the working surface (originals are backed\n" - "up). Inspect *_refinement_convergence.png first to pick the best one.")) - accept = widgets.Container(layout='vertical', labels=True) - accept.native.layout().setSpacing(5) - accept.native.layout().setContentsMargins(3, 3, 3, 3) - self.accept_step_input = widgets.SpinBox( - value=1, min=1, max=50, label='Iteration to Accept') - accept.extend([self.accept_step_input]) - inner_layout.addWidget(accept.native) - self.accept_btn = QPushButton('Accept Iteration') + "Promote one iteration per component to be the working surface (originals\n" + "are backed up). Inspect *_refinement_convergence.png first; IMM and OMM\n" + "converge differently, so you can accept a different iteration for each.")) + # One step spinbox per component, rebuilt from the *_refined_iter* files. + self.accept_container = widgets.Container(layout='vertical', labels=True) + self.accept_container.native.layout().setSpacing(5) + self.accept_container.native.layout().setContentsMargins(3, 3, 3, 3) + inner_layout.addWidget(self.accept_container.native) + self._component_steps = {} + + self.refresh_btn = QPushButton('Refresh Components') + self.refresh_btn.clicked.connect(self._refresh_accept_components) + inner_layout.addWidget(self.refresh_btn) + self.accept_btn = QPushButton('Accept Iterations') self.accept_btn.clicked.connect(self._accept_iteration) inner_layout.addWidget(self.accept_btn) @@ -191,6 +196,7 @@ def _on_config_loaded(self): self.laplacian_input.value = refine.get('laplacian_iterations', 5) self.laplacian_lambda_input.value = refine.get('laplacian_lambda', 0.5) self.lowpass_input.value = refine.get('lowpass_sigma', 0) + self._refresh_accept_components() except Exception as e: print(f"[RefinementWidget] Error in _on_config_loaded: {e}") @@ -364,6 +370,7 @@ def _run_refinement_worker(self, job_data): self.status.update_status( 'Refinement complete. Inspect *_refinement_convergence.png, then accept an iteration.') print("===== Mesh refinement complete. =====") + QTimer.singleShot(0, self._refresh_accept_components) except Exception as e: self.status.update_status(f'Error: {e}') @@ -375,6 +382,54 @@ def _run_refinement_worker(self, job_data): # ----- Accept an iteration ----- + def _discover_refined_components(self, work_dir): + """Map component name -> sorted list of available iteration numbers. + + Refined surfaces are named ``{tomo}_..._{component}_refined_iter{N}.surface.vtp``; + the component is the token immediately before ``_refined_iter``. Iterations + are aggregated across tomograms so a component's spinbox covers every N seen. + """ + pat = re.compile(r'^(?P.+)_refined_iter(?P\d+)\.surface\.vtp$') + components = {} + for p in work_dir.glob('*_refined_iter*.surface.vtp'): + m = pat.match(p.name) + if not m: + continue + component = m.group('base').rsplit('_', 1)[-1] + components.setdefault(component, set()).add(int(m.group('n'))) + return {c: sorted(v) for c, v in sorted(components.items())} + + def _refresh_accept_components(self): + """Rebuild the per-component step spinboxes from the refined files on disk.""" + # Preserve current selections across a refresh so a rescan doesn't reset them. + prev = {c: sb.value for c, sb in self._component_steps.items()} + self.accept_container.clear() + self._component_steps = {} + + try: + _, exp_dir = self._config_path() + work_dir = resolve_work_dir(exp_dir) + except Exception: + work_dir = None + + found = self._discover_refined_components(work_dir) if work_dir else {} + if not found: + self.accept_container.append(widgets.Label( + value='No refined iterations found. Run refinement, then Refresh.')) + self.accept_btn.setEnabled(False) + return + + for component, iters in found.items(): + lo, hi = iters[0], iters[-1] + # Default to the final iteration (usually the converged one); keep the + # user's prior pick if it's still in range. + default = min(max(prev.get(component, hi), lo), hi) + sb = widgets.SpinBox(value=default, min=lo, max=hi, + label=f'{component} (iters {lo}-{hi})') + self.accept_container.append(sb) + self._component_steps[component] = sb + self.accept_btn.setEnabled(not self.is_running) + def _accept_iteration(self): if self.is_running: return @@ -388,21 +443,29 @@ def _accept_iteration(self): QMessageBox.warning(self, "No Config", f"Config not found: {config_path}") return - step = self.accept_step_input.value + if not self._component_steps: + QMessageBox.warning(self, "No Components", + "No refined components found. Run refinement, then Refresh.") + return + work_dir = resolve_work_dir(exp_dir) - if not list(work_dir.glob(f'*_refined_iter{step}.surface.vtp')): + choices = {c: sb.value for c, sb in self._component_steps.items()} + # Validate each chosen iteration exists for its component before touching files. + missing = [f"{c}: iteration {s}" for c, s in choices.items() + if not list(work_dir.glob(f'*_{c}_refined_iter{s}.surface.vtp'))] + if missing: QMessageBox.warning( self, "No Such Iteration", - f"No refined surfaces for iteration {step} found in {work_dir}.\n" - "Run refinement first, or pick an iteration that was produced.") + "These selections have no refined surface:\n " + "\n ".join(missing)) return + summary = "\n".join(f" {c}: iteration {s}" for c, s in choices.items()) confirm = QMessageBox.question( - self, "Accept Iteration", - f"Promote iteration {step} to be the working surface?\n\n" - "The original surfaces are backed up (*.orig.bak), but the other " - "refinement iterations and intermediates will be removed. This cannot " - "be undone from the GUI.", + self, "Accept Iterations", + f"Promote these iterations to be the working surfaces?\n\n{summary}\n\n" + "Originals are backed up (*.orig.bak), but the other refinement " + "iterations and intermediates will be removed. This cannot be undone " + "from the GUI.", QMessageBox.Yes | QMessageBox.No, QMessageBox.No) if confirm != QMessageBox.Yes: return @@ -412,33 +475,43 @@ def _accept_iteration(self): QMessageBox.critical(self, "morphometrics CLI not found", CLI_MISSING_MESSAGE) return - job_data = {'runner': runner, 'config_path': config_path, 'step': step} + job_data = {'runner': runner, 'config_path': config_path, 'choices': choices} self.is_running = True self.submit_btn.enabled = False self.accept_btn.setEnabled(False) - self.status.update_status(f'Accepting iteration {step}...') + self.status.update_status('Accepting iterations...') threading.Thread(target=self._accept_worker, args=(job_data,), daemon=True).start() def _accept_worker(self, job_data): try: runner = job_data['runner'] config_path = job_data['config_path'] - step = job_data['step'] + choices = job_data['choices'] work_dir = resolve_work_dir(Path(config_path).parent).resolve() - cmd = runner + [ACCEPT_REFINEMENT, str(config_path), str(step)] - print(f"--- Accepting refinement: {' '.join(map(str, cmd))} ---") - try: - subprocess.run(cmd, cwd=work_dir, check=True, text=True) - except subprocess.CalledProcessError: - self.status.update_status('Error: accept_refinement failed. See terminal.') - print("[ERROR] accept_refinement failed. Check the terminal output.") - return - - self.status.update_status( - f'Accepted iteration {step}. If it was a lightweight (xcorr) iteration, ' - 're-run Curvature before distances.') - print(f"===== Accepted refinement iteration {step}. =====") + # One accept_refinement call per component. accept_one's cleanup globs + # {basename}_refined_iter*, scoped to the accepted basename, so accepting + # one component never deletes another's iterations — order is irrelevant. + failed = [] + for component, step in choices.items(): + cmd = runner + [ACCEPT_REFINEMENT, str(config_path), str(step), + '--component', component] + print(f"--- Accepting refinement: {' '.join(map(str, cmd))} ---") + try: + subprocess.run(cmd, cwd=work_dir, check=True, text=True) + except subprocess.CalledProcessError: + failed.append(component) + print(f"[ERROR] accept_refinement failed for {component}.") + + if failed: + self.status.update_status( + f"Error accepting: {', '.join(failed)}. See terminal.") + else: + accepted = ", ".join(f"{c}={s}" for c, s in choices.items()) + self.status.update_status( + f'Accepted {accepted}. If any was a lightweight (xcorr) iteration, ' + 're-run Curvature before distances.') + print(f"===== Accepted refinement: {accepted}. =====") except Exception as e: self.status.update_status(f'Error: {e}') @@ -446,9 +519,10 @@ def _accept_worker(self, job_data): import traceback traceback.print_exc() finally: + QTimer.singleShot(0, self._refresh_accept_components) QTimer.singleShot(0, self._job_cleanup) def _job_cleanup(self): self.submit_btn.enabled = True - self.accept_btn.setEnabled(True) + self.accept_btn.setEnabled(bool(self._component_steps)) self.is_running = False diff --git a/tests/test_refinement_tab.py b/tests/test_refinement_tab.py index 47522ff..93ecccb 100644 --- a/tests/test_refinement_tab.py +++ b/tests/test_refinement_tab.py @@ -1,7 +1,7 @@ """Tests for RefinementWidget.""" import pytest from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from ruamel.yaml import YAML @@ -65,3 +65,70 @@ def test_update_config_writes_refinement_keys(self, qapp, mock_experiment_manage assert cfg["tomo_dir"].endswith("/") # work_dir must end in a separator for the CLI's string concatenation. assert cfg["work_dir"].endswith("/") + + def _setup_refined(self, w, mock_experiment_manager, tmp_path, files): + """Create refined_iter surfaces under the experiment's work dir.""" + mock_experiment_manager.work_dir.value = str(tmp_path) + mock_experiment_manager.experiment_name.currentText.return_value = "exp" + from utils.script_resolver import resolve_work_dir + work_dir = resolve_work_dir(tmp_path / "exp") + work_dir.mkdir(parents=True, exist_ok=True) + for name in files: + (work_dir / name).write_text("") + return work_dir + + def test_refresh_discovers_per_component_iterations(self, qapp, mock_experiment_manager, tmp_path): + w = self._make_widget(qapp, mock_experiment_manager) + self._setup_refined(w, mock_experiment_manager, tmp_path, [ + "tomo1_labels_IMM_refined_iter1.surface.vtp", + "tomo1_labels_IMM_refined_iter6.surface.vtp", + "tomo1_labels_OMM_refined_iter1.surface.vtp", + "tomo1_labels_OMM_refined_iter4.surface.vtp", + "tomo1_labels_OMM_refined_iter5.surface.vtp", + ]) + + w._refresh_accept_components() + + assert set(w._component_steps) == {"IMM", "OMM"} + imm, omm = w._component_steps["IMM"], w._component_steps["OMM"] + # Range spans the available iterations; default is the final (converged) one. + assert (imm.min, imm.max, imm.value) == (1, 6, 6) + assert (omm.min, omm.max, omm.value) == (1, 5, 5) + assert w.accept_btn.isEnabled() + + def test_refresh_preserves_prior_selection(self, qapp, mock_experiment_manager, tmp_path): + w = self._make_widget(qapp, mock_experiment_manager) + self._setup_refined(w, mock_experiment_manager, tmp_path, [ + "t_IMM_refined_iter1.surface.vtp", + "t_IMM_refined_iter6.surface.vtp", + ]) + w._refresh_accept_components() + w._component_steps["IMM"].value = 3 + w._refresh_accept_components() + assert w._component_steps["IMM"].value == 3 + + def test_refresh_no_files_disables_accept(self, qapp, mock_experiment_manager, tmp_path): + w = self._make_widget(qapp, mock_experiment_manager) + self._setup_refined(w, mock_experiment_manager, tmp_path, []) + w._refresh_accept_components() + assert w._component_steps == {} + assert not w.accept_btn.isEnabled() + + def test_accept_worker_runs_one_call_per_component(self, qapp, mock_experiment_manager, tmp_path): + w = self._make_widget(qapp, mock_experiment_manager) + work_dir = self._setup_refined(w, mock_experiment_manager, tmp_path, []) + config_path = work_dir / "exp_config.yml" + config_path.write_text("work_dir: x\n") + + job_data = { + "runner": ["morphometrics"], + "config_path": config_path, + "choices": {"IMM": 6, "OMM": 5}, + } + with patch("jobs.refinement_tab.subprocess.run") as run: + w._accept_worker(job_data) + + cmds = [c.args[0] for c in run.call_args_list] + assert len(cmds) == 2 + for cmd, comp, step in [(cmds[0], "IMM", "6"), (cmds[1], "OMM", "5")]: + assert cmd[-3:] == [step, "--component", comp] From 0b8f123dd304eafad92c323650119d509dccb0c7 Mon Sep 17 00:00:00 2001 From: Hemanth Kapa Date: Thu, 2 Jul 2026 13:28:33 -0700 Subject: [PATCH 2/7] Preview refined iterations in napari before accepting Add a "Preview Iterations" button to the refinement tab's Accept section that loads each *_refined_iter{N}.surface.vtp (plus iter0, the original) as napari surface layers. The existing per-component accept spinbox doubles as the scrubber: changing it toggles which iteration's layer is visible, so the shown mesh is exactly what Accept promotes. - main.py builds MeshViewer before RefinementWidget and passes it in; mesh_viewer=None (headless/tests) hides the preview buttons. - Reuses mesh_viewer._load_mesh_file for VTK loading + 3D switch. - _refresh_accept_components clears stale preview layers first, since accept deletes all but the chosen iteration. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../jobs/refinement_tab.py | 141 +++++++++++++++++- src/surface_morphometrics_gui/main.py | 12 +- tests/test_refinement_tab.py | 114 ++++++++++++++ 3 files changed, 260 insertions(+), 7 deletions(-) diff --git a/src/surface_morphometrics_gui/jobs/refinement_tab.py b/src/surface_morphometrics_gui/jobs/refinement_tab.py index ff1bfd7..a1eca96 100644 --- a/src/surface_morphometrics_gui/jobs/refinement_tab.py +++ b/src/surface_morphometrics_gui/jobs/refinement_tab.py @@ -60,9 +60,15 @@ class RefinementWidget(QWidget): both before running. """ - def __init__(self, experiment_manager): + def __init__(self, experiment_manager, mesh_viewer=None): super().__init__() self.experiment_manager = experiment_manager + # Optional MeshViewer used to preview refined iterations in napari before + # accepting one. None in headless/test paths — preview is then disabled. + self.mesh_viewer = mesh_viewer + # napari layers we created for the current preview: list of + # (component, iter_n, layer) so visibility/clear needn't re-parse names. + self._preview_layers = [] self.is_running = False main_layout = QVBoxLayout() @@ -159,6 +165,25 @@ def __init__(self, experiment_manager): self.refresh_btn = QPushButton('Refresh Components') self.refresh_btn.clicked.connect(self._refresh_accept_components) inner_layout.addWidget(self.refresh_btn) + + # Preview the refined iterations in napari before the destructive accept. + # The per-component spinbox above is the scrubber: the iteration it shows + # is the one Accept promotes. Only available when a MeshViewer was wired in. + if self.mesh_viewer is not None: + inner_layout.addWidget(QLabel( + "Preview loads each iteration (plus iter0, the original) as napari\n" + "surfaces; scrub with the spinbox above — the shown iteration is\n" + "the one Accept promotes.")) + self.preview_btn = QPushButton('Preview Iterations') + self.preview_btn.clicked.connect(self._preview_iterations) + inner_layout.addWidget(self.preview_btn) + self.clear_preview_btn = QPushButton('Clear Preview') + self.clear_preview_btn.clicked.connect(self._clear_preview) + inner_layout.addWidget(self.clear_preview_btn) + else: + self.preview_btn = None + self.clear_preview_btn = None + self.accept_btn = QPushButton('Accept Iterations') self.accept_btn.clicked.connect(self._accept_iteration) inner_layout.addWidget(self.accept_btn) @@ -317,6 +342,8 @@ def _run_refinement(self): self.is_running = True self.submit_btn.enabled = False self.accept_btn.setEnabled(False) + if self.preview_btn is not None: + self.preview_btn.setEnabled(False) self.status.update_status('Starting...') self.status.update_progress(0) threading.Thread(target=self._run_refinement_worker, args=(job_data,), daemon=True).start() @@ -403,6 +430,9 @@ def _refresh_accept_components(self): """Rebuild the per-component step spinboxes from the refined files on disk.""" # Preserve current selections across a refresh so a rescan doesn't reset them. prev = {c: sb.value for c, sb in self._component_steps.items()} + # The file set is about to change (refine/accept just ran); drop stale + # preview layers so they can't outlive the iterations they represent. + self._clear_preview() self.accept_container.clear() self._component_steps = {} @@ -417,6 +447,8 @@ def _refresh_accept_components(self): self.accept_container.append(widgets.Label( value='No refined iterations found. Run refinement, then Refresh.')) self.accept_btn.setEnabled(False) + if self.preview_btn is not None: + self.preview_btn.setEnabled(False) return for component, iters in found.items(): @@ -426,9 +458,112 @@ def _refresh_accept_components(self): default = min(max(prev.get(component, hi), lo), hi) sb = widgets.SpinBox(value=default, min=lo, max=hi, label=f'{component} (iters {lo}-{hi})') + # Scrubber: when a preview is loaded, changing the step shows that + # iteration's layer and hides the rest for this component. + sb.changed.connect(lambda _=None, c=component: self._on_step_changed(c)) self.accept_container.append(sb) self._component_steps[component] = sb self.accept_btn.setEnabled(not self.is_running) + if self.preview_btn is not None: + self.preview_btn.setEnabled(not self.is_running) + + # ----- Preview iterations in napari ----- + + def _preview_iterations(self): + """Load every refined iteration (plus iter0 = the original surface) into + napari as surface layers, showing only the spinbox-selected iteration per + component. The accept spinbox then scrubs iterations via visibility.""" + if self.mesh_viewer is None or not self._component_steps: + return + + self._clear_preview() + + try: + _, exp_dir = self._config_path() + work_dir = resolve_work_dir(exp_dir) + except Exception as e: + QMessageBox.warning(self, "Preview Failed", f"Could not resolve work dir: {e}") + return + + pat = re.compile(r'^(?P.+)_refined_iter(?P\d+)\.surface\.vtp$') + loaded = 0 + for component in self._component_steps: + # (base, iter_n, path) for this component across all tomogram basenames. + files = [] + bases = set() + for p in sorted(work_dir.glob(f'*_{component}_refined_iter*.surface.vtp')): + m = pat.match(p.name) + if not m: + continue + files.append((m.group('base'), int(m.group('n')), p)) + bases.add(m.group('base')) + # iter0 = the still-canonical original surface for each base. + for base in sorted(bases): + orig = work_dir / f'{base}.surface.vtp' + if orig.exists(): + files.append((base, 0, orig)) + + multi_base = len(bases) > 1 + for base, n, path in files: + suffix = f':{base}' if multi_base else '' + name = f'refine-preview:{component}:iter{n}{suffix}' + layer = self._add_preview_layer(str(path), name) + if layer is not None: + self._preview_layers.append((component, n, layer)) + loaded += 1 + + if not loaded: + QMessageBox.information( + self, "Nothing to Preview", + "No refined iteration surfaces were found for the current components.") + return + + for component, sb in self._component_steps.items(): + self._on_step_changed(component) + self.mesh_viewer.viewer.reset_view() + + def _add_preview_layer(self, path, name): + """Load a .vtp via the MeshViewer and return the created (renamed) layer.""" + viewer = self.mesh_viewer.viewer + before = set(viewer.layers) + try: + self.mesh_viewer._load_mesh_file(path) + except Exception as e: + print(f"[RefinementWidget] Failed to preview {path}: {e}") + return None + new = [l for l in viewer.layers if l not in before] + if not new: + return None + layer = new[-1] + layer.name = name + return layer + + def _on_step_changed(self, component): + """Show only the selected iteration's layer(s) for this component.""" + sb = self._component_steps.get(component) + if sb is None or not self._preview_layers: + return + target = sb.value + for comp, n, layer in self._preview_layers: + if comp == component: + try: + layer.visible = (n == target) + except Exception: + pass + + def _clear_preview(self): + """Remove all preview layers we created from the napari viewer.""" + if self.mesh_viewer is None: + self._preview_layers = [] + return + layers = self.mesh_viewer.viewer.layers + for _comp, _n, layer in self._preview_layers: + try: + if layer in layers: + layers.remove(layer) + except Exception: + pass + self._preview_layers = [] def _accept_iteration(self): if self.is_running: @@ -479,6 +614,8 @@ def _accept_iteration(self): self.is_running = True self.submit_btn.enabled = False self.accept_btn.setEnabled(False) + if self.preview_btn is not None: + self.preview_btn.setEnabled(False) self.status.update_status('Accepting iterations...') threading.Thread(target=self._accept_worker, args=(job_data,), daemon=True).start() @@ -525,4 +662,6 @@ def _accept_worker(self, job_data): def _job_cleanup(self): self.submit_btn.enabled = True self.accept_btn.setEnabled(bool(self._component_steps)) + if self.preview_btn is not None: + self.preview_btn.setEnabled(bool(self._component_steps)) self.is_running = False diff --git a/src/surface_morphometrics_gui/main.py b/src/surface_morphometrics_gui/main.py index 949bb37..46ea7fc 100644 --- a/src/surface_morphometrics_gui/main.py +++ b/src/surface_morphometrics_gui/main.py @@ -83,22 +83,22 @@ def main(): # Setup responsive layout setup_responsive_layout(viewer) + # Create mesh viewer widget (before RefinementWidget, which reuses it to + # preview refined iterations in napari before accepting one). + mesh_viewer = MeshViewer(viewer) + # Create widgets experiment_manager = ExperimentManager(viewer) mesh_widget = MeshGenerationWidget(experiment_manager) pycurv_widget = PyCurvWidget(experiment_manager=experiment_manager) - refinement_widget = RefinementWidget(experiment_manager) + refinement_widget = RefinementWidget(experiment_manager, mesh_viewer=mesh_viewer) distance_widget = DistanceOrientationWidget(experiment_manager) thickness_widget = ThicknessWidget(experiment_manager) # (Mesh completion connection set after dock widgets are created below) # Create tomoslice plugin tomoslice = TomoslicePlugin(viewer, experiment_manager) - - - # Create mesh viewer widget - mesh_viewer = MeshViewer(viewer) - + # Setup and add dock widgets with proper sizing dw1 = viewer.window.add_dock_widget(experiment_manager, name='Experiment Manager', area='right') diff --git a/tests/test_refinement_tab.py b/tests/test_refinement_tab.py index 93ecccb..1199956 100644 --- a/tests/test_refinement_tab.py +++ b/tests/test_refinement_tab.py @@ -132,3 +132,117 @@ def test_accept_worker_runs_one_call_per_component(self, qapp, mock_experiment_m assert len(cmds) == 2 for cmd, comp, step in [(cmds[0], "IMM", "6"), (cmds[1], "OMM", "5")]: assert cmd[-3:] == [step, "--component", comp] + + +class _FakeLayer: + def __init__(self, path): + self.path = path + self.name = path + self.visible = True + + +class _FakeLayerViewer: + def __init__(self): + self.layers = [] + self.reset_view_called = 0 + + def reset_view(self): + self.reset_view_called += 1 + + +class _FakeMeshViewer: + """Minimal stand-in: _load_mesh_file appends a surface layer to the viewer.""" + def __init__(self): + self.viewer = _FakeLayerViewer() + self.loaded = [] + + def _load_mesh_file(self, path): + self.loaded.append(path) + self.viewer.layers.append(_FakeLayer(path)) + + +@pytest.mark.gui +class TestRefinementPreview: + def _make_widget(self, mock_experiment_manager, mesh_viewer): + from jobs.refinement_tab import RefinementWidget + mock_experiment_manager.config_loaded = MagicMock() + mock_experiment_manager.current_config = None + return RefinementWidget(mock_experiment_manager, mesh_viewer=mesh_viewer) + + def _setup_refined(self, mock_experiment_manager, tmp_path, files): + mock_experiment_manager.work_dir.value = str(tmp_path) + mock_experiment_manager.experiment_name.currentText.return_value = "exp" + from utils.script_resolver import resolve_work_dir + work_dir = resolve_work_dir(tmp_path / "exp") + work_dir.mkdir(parents=True, exist_ok=True) + for name in files: + (work_dir / name).write_text("") + return work_dir + + def test_preview_builds_layers_including_iter0(self, qapp, mock_experiment_manager, tmp_path): + mv = _FakeMeshViewer() + w = self._make_widget(mock_experiment_manager, mv) + self._setup_refined(mock_experiment_manager, tmp_path, [ + "t_IMM.surface.vtp", # iter0 = original + "t_IMM_refined_iter1.surface.vtp", + "t_IMM_refined_iter6.surface.vtp", + ]) + w._refresh_accept_components() + w._preview_iterations() + + names = {l.name for _c, _n, l in w._preview_layers} + assert names == { + "refine-preview:IMM:iter0", + "refine-preview:IMM:iter1", + "refine-preview:IMM:iter6", + } + # Default spinbox value (final iter) is the only visible layer. + visible = {n for _c, n, l in w._preview_layers if l.visible} + assert visible == {6} + assert mv.viewer.reset_view_called == 1 + + def test_spinbox_scrubs_visibility(self, qapp, mock_experiment_manager, tmp_path): + mv = _FakeMeshViewer() + w = self._make_widget(mock_experiment_manager, mv) + self._setup_refined(mock_experiment_manager, tmp_path, [ + "t_IMM.surface.vtp", + "t_IMM_refined_iter1.surface.vtp", + "t_IMM_refined_iter6.surface.vtp", + ]) + w._refresh_accept_components() + w._preview_iterations() + + w._component_steps["IMM"].value = 1 # emits changed -> _on_step_changed + visible = {n for _c, n, l in w._preview_layers if l.visible} + assert visible == {1} + + def test_clear_preview_removes_layers(self, qapp, mock_experiment_manager, tmp_path): + mv = _FakeMeshViewer() + w = self._make_widget(mock_experiment_manager, mv) + self._setup_refined(mock_experiment_manager, tmp_path, [ + "t_IMM_refined_iter1.surface.vtp", + ]) + w._refresh_accept_components() + w._preview_iterations() + assert mv.viewer.layers + w._clear_preview() + assert w._preview_layers == [] + assert mv.viewer.layers == [] + + def test_refresh_clears_stale_preview(self, qapp, mock_experiment_manager, tmp_path): + mv = _FakeMeshViewer() + w = self._make_widget(mock_experiment_manager, mv) + self._setup_refined(mock_experiment_manager, tmp_path, [ + "t_IMM_refined_iter1.surface.vtp", + ]) + w._refresh_accept_components() + w._preview_iterations() + assert w._preview_layers + w._refresh_accept_components() + assert w._preview_layers == [] + assert mv.viewer.layers == [] + + def test_no_viewer_disables_preview(self, qapp, mock_experiment_manager): + w = self._make_widget(mock_experiment_manager, None) + assert w.preview_btn is None + assert w.clear_preview_btn is None From 0c87eb0889237edf4e9fd93df46b462278475406 Mon Sep 17 00:00:00 2001 From: Hemanth Kapa Date: Thu, 2 Jul 2026 14:47:32 -0700 Subject: [PATCH 3/7] Load refinement previews flat (no scalar coloring) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refined *.surface.vtp files carry a noisy per-vertex scalar array; the Mesh Viewer auto-colored previews by it (speckled viridis), which is useless for judging shape. Add a flat=True mode to _load_mesh_file that skips scalar auto-coloring (marks the layer initialized, omits source_vtp_path) and shows a matte gray surface — ambient occlusion still gives depth. Preview layers now load flat. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../jobs/refinement_tab.py | 15 +++------ .../plugins/mesh_viewer.py | 31 ++++++++++++++----- tests/test_refinement_tab.py | 14 ++++++--- 3 files changed, 38 insertions(+), 22 deletions(-) diff --git a/src/surface_morphometrics_gui/jobs/refinement_tab.py b/src/surface_morphometrics_gui/jobs/refinement_tab.py index a1eca96..47d6f3f 100644 --- a/src/surface_morphometrics_gui/jobs/refinement_tab.py +++ b/src/surface_morphometrics_gui/jobs/refinement_tab.py @@ -523,20 +523,15 @@ def _preview_iterations(self): self.mesh_viewer.viewer.reset_view() def _add_preview_layer(self, path, name): - """Load a .vtp via the MeshViewer and return the created (renamed) layer.""" - viewer = self.mesh_viewer.viewer - before = set(viewer.layers) + """Load a .vtp as a flat gray preview surface and return the layer. + + Loaded flat (no per-vertex scalar coloring) so previews show shape, not + the noisy scalar arrays some refined surfaces carry.""" try: - self.mesh_viewer._load_mesh_file(path) + return self.mesh_viewer._load_mesh_file(path, name=name, flat=True) except Exception as e: print(f"[RefinementWidget] Failed to preview {path}: {e}") return None - new = [l for l in viewer.layers if l not in before] - if not new: - return None - layer = new[-1] - layer.name = name - return layer def _on_step_changed(self, component): """Show only the selected iteration's layer(s) for this component.""" diff --git a/src/surface_morphometrics_gui/plugins/mesh_viewer.py b/src/surface_morphometrics_gui/plugins/mesh_viewer.py index 8ce78a6..ea00ba2 100644 --- a/src/surface_morphometrics_gui/plugins/mesh_viewer.py +++ b/src/surface_morphometrics_gui/plugins/mesh_viewer.py @@ -191,8 +191,15 @@ def _on_load_mesh_clicked(self): self._load_mesh_file(filepath) - def _load_mesh_file(self, filepath): - """Load a mesh file using VTK and add it to napari.""" + def _load_mesh_file(self, filepath, name=None, flat=False): + """Load a mesh file using VTK and add it to napari. + + flat=True loads the mesh as a matte gray surface for shape comparison: + it skips the per-vertex scalar auto-coloring (marks the layer already + initialized and omits ``source_vtp_path``), so the noisy scalar arrays + some VTPs carry don't paint the surface. Ambient occlusion still applies, + giving depth. Returns the created layer (or None on failure). + """ ext = os.path.splitext(filepath)[1].lower() reader = None @@ -237,18 +244,26 @@ def _load_mesh_file(self, filepath): values = np.ones(len(vertices)) mesh_tuple = (vertices, faces, values) - name = os.path.splitext(os.path.basename(filepath))[0] - # Only tag VTP files as the source path; other formats (PLY, STL, OBJ) - # cannot be re-read by vtkXMLPolyDataReader and would cause an XML parse - # error at byte 0 when _initialize_vtp_layer tries to load them. - metadata = {'source_vtp_path': filepath} if ext == '.vtp' else {} + if name is None: + name = os.path.splitext(os.path.basename(filepath))[0] + add_kwargs = {'name': name} + if flat: + # Skip scalar auto-coloring (see docstring); show a matte gray surface. + add_kwargs['metadata'] = {'vtp_initialized': True} + add_kwargs['colormap'] = 'gray' + else: + # Only tag VTP files as the source path; other formats (PLY, STL, OBJ) + # cannot be re-read by vtkXMLPolyDataReader and would cause an XML parse + # error at byte 0 when _initialize_vtp_layer tries to load them. + add_kwargs['metadata'] = {'source_vtp_path': filepath} if ext == '.vtp' else {} # Surface layers require 3D display mode; switch automatically. if self.viewer.dims.ndisplay != 3: self.viewer.dims.ndisplay = 3 - self.viewer.add_surface(mesh_tuple, name=name, metadata=metadata) + layer = self.viewer.add_surface(mesh_tuple, **add_kwargs) self.viewer.reset_view() + return layer def _is_vtp_surface_layer(self, layer): """Checks if a layer is a Surface derived from a VTP file.""" diff --git a/tests/test_refinement_tab.py b/tests/test_refinement_tab.py index 1199956..b0e22ed 100644 --- a/tests/test_refinement_tab.py +++ b/tests/test_refinement_tab.py @@ -156,9 +156,13 @@ def __init__(self): self.viewer = _FakeLayerViewer() self.loaded = [] - def _load_mesh_file(self, path): - self.loaded.append(path) - self.viewer.layers.append(_FakeLayer(path)) + def _load_mesh_file(self, path, name=None, flat=False): + self.loaded.append((path, name, flat)) + layer = _FakeLayer(path) + if name is not None: + layer.name = name + self.viewer.layers.append(layer) + return layer @pytest.mark.gui @@ -199,7 +203,9 @@ def test_preview_builds_layers_including_iter0(self, qapp, mock_experiment_manag # Default spinbox value (final iter) is the only visible layer. visible = {n for _c, n, l in w._preview_layers if l.visible} assert visible == {6} - assert mv.viewer.reset_view_called == 1 + assert mv.viewer.reset_view_called >= 1 + # Preview surfaces load flat (no scalar coloring) for shape comparison. + assert all(flat for _p, _n, flat in mv.loaded) def test_spinbox_scrubs_visibility(self, qapp, mock_experiment_manager, tmp_path): mv = _FakeMeshViewer() From f1ef6e655953d97f34389307e67c5934391960e0 Mon Sep 17 00:00:00 2001 From: Hemanth Kapa Date: Mon, 6 Jul 2026 10:35:32 -0700 Subject: [PATCH 4/7] =?UTF-8?q?Accept=20refinement=20per=20tomogram=C3=97s?= =?UTF-8?q?urface=20instead=20of=20per=20component.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Different tomograms can now pick different iterations; accept calls pass --tomogram and --component, and the UI lists one spinbox per surface basename. Co-authored-by: Cursor --- .../jobs/refinement_tab.py | 159 +++++++++--------- tests/test_refinement_tab.py | 56 +++--- 2 files changed, 120 insertions(+), 95 deletions(-) diff --git a/src/surface_morphometrics_gui/jobs/refinement_tab.py b/src/surface_morphometrics_gui/jobs/refinement_tab.py index 47d6f3f..78e93d2 100644 --- a/src/surface_morphometrics_gui/jobs/refinement_tab.py +++ b/src/surface_morphometrics_gui/jobs/refinement_tab.py @@ -6,7 +6,7 @@ from pathlib import Path from magicgui import widgets -from qtpy.QtCore import QTimer +from qtpy.QtCore import Qt, QTimer from ruamel.yaml import YAML from qtpy.QtWidgets import ( QLabel, @@ -67,7 +67,7 @@ def __init__(self, experiment_manager, mesh_viewer=None): # accepting one. None in headless/test paths — preview is then disabled. self.mesh_viewer = mesh_viewer # napari layers we created for the current preview: list of - # (component, iter_n, layer) so visibility/clear needn't re-parse names. + # (basename, iter_n, layer) so visibility/clear needn't re-parse names. self._preview_layers = [] self.is_running = False @@ -152,17 +152,24 @@ def __init__(self, experiment_manager, mesh_viewer=None): # --- Accept an iteration (destructive: promotes one, removes the rest) --- inner_layout.addWidget(QLabel("Accept Iteration")) inner_layout.addWidget(QLabel( - "Promote one iteration per component to be the working surface (originals\n" - "are backed up). Inspect *_refinement_convergence.png first; IMM and OMM\n" - "converge differently, so you can accept a different iteration for each.")) - # One step spinbox per component, rebuilt from the *_refined_iter* files. + "Promote one iteration per surface (one row per tomogram × membrane;\n" + "originals are backed up). Inspect *_refinement_convergence.png first;\n" + "each surface can converge at a different iteration.")) + # One step spinbox per surface basename, rebuilt from *_refined_iter* files. self.accept_container = widgets.Container(layout='vertical', labels=True) self.accept_container.native.layout().setSpacing(5) self.accept_container.native.layout().setContentsMargins(3, 3, 3, 3) - inner_layout.addWidget(self.accept_container.native) - self._component_steps = {} - - self.refresh_btn = QPushButton('Refresh Components') + accept_scroll = QScrollArea() + accept_scroll.setWidgetResizable(True) + accept_scroll.setHorizontalScrollBarPolicy(Qt.ScrollBarAlwaysOff) + accept_scroll.setFrameShape(QScrollArea.NoFrame) + accept_scroll.setWidget(self.accept_container.native) + accept_scroll.setMinimumHeight(80) + accept_scroll.setMaximumHeight(280) + inner_layout.addWidget(accept_scroll) + self._surface_steps = {} + + self.refresh_btn = QPushButton('Refresh Surfaces') self.refresh_btn.clicked.connect(self._refresh_accept_components) inner_layout.addWidget(self.refresh_btn) @@ -172,8 +179,8 @@ def __init__(self, experiment_manager, mesh_viewer=None): if self.mesh_viewer is not None: inner_layout.addWidget(QLabel( "Preview loads each iteration (plus iter0, the original) as napari\n" - "surfaces; scrub with the spinbox above — the shown iteration is\n" - "the one Accept promotes.")) + "surfaces per row above; scrub a spinbox — the shown iteration is\n" + "the one Accept promotes for that surface.")) self.preview_btn = QPushButton('Preview Iterations') self.preview_btn.clicked.connect(self._preview_iterations) inner_layout.addWidget(self.preview_btn) @@ -409,32 +416,38 @@ def _run_refinement_worker(self, job_data): # ----- Accept an iteration ----- - def _discover_refined_components(self, work_dir): - """Map component name -> sorted list of available iteration numbers. + def _discover_refined_surfaces(self, work_dir): + """Map surface basename -> sorted list of available iteration numbers. - Refined surfaces are named ``{tomo}_..._{component}_refined_iter{N}.surface.vtp``; - the component is the token immediately before ``_refined_iter``. Iterations - are aggregated across tomograms so a component's spinbox covers every N seen. + Refined surfaces are named ``{basename}_refined_iter{N}.surface.vtp`` where + ``basename`` is typically ``{tomogram}_{component}`` (e.g. ``TE1_IMM``). + Each basename gets its own spinbox so tomograms can accept different steps. """ pat = re.compile(r'^(?P.+)_refined_iter(?P\d+)\.surface\.vtp$') - components = {} + surfaces = {} for p in work_dir.glob('*_refined_iter*.surface.vtp'): m = pat.match(p.name) if not m: continue - component = m.group('base').rsplit('_', 1)[-1] - components.setdefault(component, set()).add(int(m.group('n'))) - return {c: sorted(v) for c, v in sorted(components.items())} + surfaces.setdefault(m.group('base'), set()).add(int(m.group('n'))) + return {b: sorted(v) for b, v in sorted(surfaces.items())} + + @staticmethod + def _basename_filters(basename): + """Split ``{tomogram}_{component}`` for ``accept_refinement`` CLI filters.""" + component = basename.rsplit('_', 1)[-1] + tomogram = basename[:-(len(component) + 1)] + return tomogram, component def _refresh_accept_components(self): - """Rebuild the per-component step spinboxes from the refined files on disk.""" + """Rebuild the per-surface step spinboxes from the refined files on disk.""" # Preserve current selections across a refresh so a rescan doesn't reset them. - prev = {c: sb.value for c, sb in self._component_steps.items()} + prev = {b: sb.value for b, sb in self._surface_steps.items()} # The file set is about to change (refine/accept just ran); drop stale # preview layers so they can't outlive the iterations they represent. self._clear_preview() self.accept_container.clear() - self._component_steps = {} + self._surface_steps = {} try: _, exp_dir = self._config_path() @@ -442,7 +455,7 @@ def _refresh_accept_components(self): except Exception: work_dir = None - found = self._discover_refined_components(work_dir) if work_dir else {} + found = self._discover_refined_surfaces(work_dir) if work_dir else {} if not found: self.accept_container.append(widgets.Label( value='No refined iterations found. Run refinement, then Refresh.')) @@ -451,18 +464,18 @@ def _refresh_accept_components(self): self.preview_btn.setEnabled(False) return - for component, iters in found.items(): + for basename, iters in found.items(): lo, hi = iters[0], iters[-1] # Default to the final iteration (usually the converged one); keep the # user's prior pick if it's still in range. - default = min(max(prev.get(component, hi), lo), hi) + default = min(max(prev.get(basename, hi), lo), hi) sb = widgets.SpinBox(value=default, min=lo, max=hi, - label=f'{component} (iters {lo}-{hi})') + label=f'{basename} (iters {lo}-{hi})') # Scrubber: when a preview is loaded, changing the step shows that - # iteration's layer and hides the rest for this component. - sb.changed.connect(lambda _=None, c=component: self._on_step_changed(c)) + # iteration's layer and hides the rest for this surface. + sb.changed.connect(lambda _=None, b=basename: self._on_step_changed(b)) self.accept_container.append(sb) - self._component_steps[component] = sb + self._surface_steps[basename] = sb self.accept_btn.setEnabled(not self.is_running) if self.preview_btn is not None: self.preview_btn.setEnabled(not self.is_running) @@ -472,8 +485,8 @@ def _refresh_accept_components(self): def _preview_iterations(self): """Load every refined iteration (plus iter0 = the original surface) into napari as surface layers, showing only the spinbox-selected iteration per - component. The accept spinbox then scrubs iterations via visibility.""" - if self.mesh_viewer is None or not self._component_steps: + surface basename. Each accept spinbox scrubs its own layers via visibility.""" + if self.mesh_viewer is None or not self._surface_steps: return self._clear_preview() @@ -487,39 +500,32 @@ def _preview_iterations(self): pat = re.compile(r'^(?P.+)_refined_iter(?P\d+)\.surface\.vtp$') loaded = 0 - for component in self._component_steps: - # (base, iter_n, path) for this component across all tomogram basenames. + for basename in self._surface_steps: files = [] - bases = set() - for p in sorted(work_dir.glob(f'*_{component}_refined_iter*.surface.vtp')): + for p in sorted(work_dir.glob(f'{basename}_refined_iter*.surface.vtp')): m = pat.match(p.name) if not m: continue - files.append((m.group('base'), int(m.group('n')), p)) - bases.add(m.group('base')) - # iter0 = the still-canonical original surface for each base. - for base in sorted(bases): - orig = work_dir / f'{base}.surface.vtp' - if orig.exists(): - files.append((base, 0, orig)) - - multi_base = len(bases) > 1 - for base, n, path in files: - suffix = f':{base}' if multi_base else '' - name = f'refine-preview:{component}:iter{n}{suffix}' + files.append((int(m.group('n')), p)) + orig = work_dir / f'{basename}.surface.vtp' + if orig.exists(): + files.append((0, orig)) + + for n, path in files: + name = f'refine-preview:{basename}:iter{n}' layer = self._add_preview_layer(str(path), name) if layer is not None: - self._preview_layers.append((component, n, layer)) + self._preview_layers.append((basename, n, layer)) loaded += 1 if not loaded: QMessageBox.information( self, "Nothing to Preview", - "No refined iteration surfaces were found for the current components.") + "No refined iteration surfaces were found for the current surfaces.") return - for component, sb in self._component_steps.items(): - self._on_step_changed(component) + for basename in self._surface_steps: + self._on_step_changed(basename) self.mesh_viewer.viewer.reset_view() def _add_preview_layer(self, path, name): @@ -533,14 +539,14 @@ def _add_preview_layer(self, path, name): print(f"[RefinementWidget] Failed to preview {path}: {e}") return None - def _on_step_changed(self, component): - """Show only the selected iteration's layer(s) for this component.""" - sb = self._component_steps.get(component) + def _on_step_changed(self, basename): + """Show only the selected iteration's layer(s) for this surface.""" + sb = self._surface_steps.get(basename) if sb is None or not self._preview_layers: return target = sb.value - for comp, n, layer in self._preview_layers: - if comp == component: + for base, n, layer in self._preview_layers: + if base == basename: try: layer.visible = (n == target) except Exception: @@ -573,23 +579,23 @@ def _accept_iteration(self): QMessageBox.warning(self, "No Config", f"Config not found: {config_path}") return - if not self._component_steps: - QMessageBox.warning(self, "No Components", - "No refined components found. Run refinement, then Refresh.") + if not self._surface_steps: + QMessageBox.warning(self, "No Surfaces", + "No refined surfaces found. Run refinement, then Refresh.") return work_dir = resolve_work_dir(exp_dir) - choices = {c: sb.value for c, sb in self._component_steps.items()} - # Validate each chosen iteration exists for its component before touching files. - missing = [f"{c}: iteration {s}" for c, s in choices.items() - if not list(work_dir.glob(f'*_{c}_refined_iter{s}.surface.vtp'))] + choices = {b: sb.value for b, sb in self._surface_steps.items()} + # Validate each chosen iteration exists for its surface before touching files. + missing = [f"{b}: iteration {s}" for b, s in choices.items() + if not (work_dir / f'{b}_refined_iter{s}.surface.vtp').exists()] if missing: QMessageBox.warning( self, "No Such Iteration", "These selections have no refined surface:\n " + "\n ".join(missing)) return - summary = "\n".join(f" {c}: iteration {s}" for c, s in choices.items()) + summary = "\n".join(f" {b}: iteration {s}" for b, s in choices.items()) confirm = QMessageBox.question( self, "Accept Iterations", f"Promote these iterations to be the working surfaces?\n\n{summary}\n\n" @@ -621,25 +627,26 @@ def _accept_worker(self, job_data): choices = job_data['choices'] work_dir = resolve_work_dir(Path(config_path).parent).resolve() - # One accept_refinement call per component. accept_one's cleanup globs - # {basename}_refined_iter*, scoped to the accepted basename, so accepting - # one component never deletes another's iterations — order is irrelevant. + # One accept_refinement call per surface basename, scoped with + # --tomogram and --component so each tomogram×membrane can pick a + # different iteration without affecting the others. failed = [] - for component, step in choices.items(): + for basename, step in choices.items(): + tomogram, component = self._basename_filters(basename) cmd = runner + [ACCEPT_REFINEMENT, str(config_path), str(step), - '--component', component] + '--tomogram', tomogram, '--component', component] print(f"--- Accepting refinement: {' '.join(map(str, cmd))} ---") try: subprocess.run(cmd, cwd=work_dir, check=True, text=True) except subprocess.CalledProcessError: - failed.append(component) - print(f"[ERROR] accept_refinement failed for {component}.") + failed.append(basename) + print(f"[ERROR] accept_refinement failed for {basename}.") if failed: self.status.update_status( f"Error accepting: {', '.join(failed)}. See terminal.") else: - accepted = ", ".join(f"{c}={s}" for c, s in choices.items()) + accepted = ", ".join(f"{b}={s}" for b, s in choices.items()) self.status.update_status( f'Accepted {accepted}. If any was a lightweight (xcorr) iteration, ' 're-run Curvature before distances.') @@ -656,7 +663,7 @@ def _accept_worker(self, job_data): def _job_cleanup(self): self.submit_btn.enabled = True - self.accept_btn.setEnabled(bool(self._component_steps)) + self.accept_btn.setEnabled(bool(self._surface_steps)) if self.preview_btn is not None: - self.preview_btn.setEnabled(bool(self._component_steps)) + self.preview_btn.setEnabled(bool(self._surface_steps)) self.is_running = False diff --git a/tests/test_refinement_tab.py b/tests/test_refinement_tab.py index b0e22ed..02d5dfa 100644 --- a/tests/test_refinement_tab.py +++ b/tests/test_refinement_tab.py @@ -77,7 +77,7 @@ def _setup_refined(self, w, mock_experiment_manager, tmp_path, files): (work_dir / name).write_text("") return work_dir - def test_refresh_discovers_per_component_iterations(self, qapp, mock_experiment_manager, tmp_path): + def test_refresh_discovers_per_surface_iterations(self, qapp, mock_experiment_manager, tmp_path): w = self._make_widget(qapp, mock_experiment_manager) self._setup_refined(w, mock_experiment_manager, tmp_path, [ "tomo1_labels_IMM_refined_iter1.surface.vtp", @@ -89,13 +89,26 @@ def test_refresh_discovers_per_component_iterations(self, qapp, mock_experiment_ w._refresh_accept_components() - assert set(w._component_steps) == {"IMM", "OMM"} - imm, omm = w._component_steps["IMM"], w._component_steps["OMM"] - # Range spans the available iterations; default is the final (converged) one. + assert set(w._surface_steps) == {"tomo1_labels_IMM", "tomo1_labels_OMM"} + imm, omm = w._surface_steps["tomo1_labels_IMM"], w._surface_steps["tomo1_labels_OMM"] assert (imm.min, imm.max, imm.value) == (1, 6, 6) assert (omm.min, omm.max, omm.value) == (1, 5, 5) assert w.accept_btn.isEnabled() + def test_refresh_discovers_all_tomograms_separately(self, qapp, mock_experiment_manager, tmp_path): + w = self._make_widget(qapp, mock_experiment_manager) + self._setup_refined(w, mock_experiment_manager, tmp_path, [ + "TE1_IMM_refined_iter1.surface.vtp", + "TE1_IMM_refined_iter6.surface.vtp", + "TE2_IMM_refined_iter1.surface.vtp", + "TE2_IMM_refined_iter4.surface.vtp", + ]) + w._refresh_accept_components() + + assert set(w._surface_steps) == {"TE1_IMM", "TE2_IMM"} + assert w._surface_steps["TE1_IMM"].value == 6 + assert w._surface_steps["TE2_IMM"].value == 4 + def test_refresh_preserves_prior_selection(self, qapp, mock_experiment_manager, tmp_path): w = self._make_widget(qapp, mock_experiment_manager) self._setup_refined(w, mock_experiment_manager, tmp_path, [ @@ -103,18 +116,18 @@ def test_refresh_preserves_prior_selection(self, qapp, mock_experiment_manager, "t_IMM_refined_iter6.surface.vtp", ]) w._refresh_accept_components() - w._component_steps["IMM"].value = 3 + w._surface_steps["t_IMM"].value = 3 w._refresh_accept_components() - assert w._component_steps["IMM"].value == 3 + assert w._surface_steps["t_IMM"].value == 3 def test_refresh_no_files_disables_accept(self, qapp, mock_experiment_manager, tmp_path): w = self._make_widget(qapp, mock_experiment_manager) self._setup_refined(w, mock_experiment_manager, tmp_path, []) w._refresh_accept_components() - assert w._component_steps == {} + assert w._surface_steps == {} assert not w.accept_btn.isEnabled() - def test_accept_worker_runs_one_call_per_component(self, qapp, mock_experiment_manager, tmp_path): + def test_accept_worker_runs_one_call_per_surface(self, qapp, mock_experiment_manager, tmp_path): w = self._make_widget(qapp, mock_experiment_manager) work_dir = self._setup_refined(w, mock_experiment_manager, tmp_path, []) config_path = work_dir / "exp_config.yml" @@ -123,15 +136,20 @@ def test_accept_worker_runs_one_call_per_component(self, qapp, mock_experiment_m job_data = { "runner": ["morphometrics"], "config_path": config_path, - "choices": {"IMM": 6, "OMM": 5}, + "choices": {"TE1_IMM": 6, "TE1_OMM": 5, "TE2_IMM": 4}, } with patch("jobs.refinement_tab.subprocess.run") as run: w._accept_worker(job_data) cmds = [c.args[0] for c in run.call_args_list] - assert len(cmds) == 2 - for cmd, comp, step in [(cmds[0], "IMM", "6"), (cmds[1], "OMM", "5")]: - assert cmd[-3:] == [step, "--component", comp] + assert len(cmds) == 3 + expected = [ + (["6", "--tomogram", "TE1", "--component", "IMM"]), + (["5", "--tomogram", "TE1", "--component", "OMM"]), + (["4", "--tomogram", "TE2", "--component", "IMM"]), + ] + for cmd, exp_tail in zip(cmds, expected): + assert cmd[-5:] == exp_tail class _FakeLayer: @@ -194,14 +212,14 @@ def test_preview_builds_layers_including_iter0(self, qapp, mock_experiment_manag w._refresh_accept_components() w._preview_iterations() - names = {l.name for _c, _n, l in w._preview_layers} + names = {l.name for _b, _n, l in w._preview_layers} assert names == { - "refine-preview:IMM:iter0", - "refine-preview:IMM:iter1", - "refine-preview:IMM:iter6", + "refine-preview:t_IMM:iter0", + "refine-preview:t_IMM:iter1", + "refine-preview:t_IMM:iter6", } # Default spinbox value (final iter) is the only visible layer. - visible = {n for _c, n, l in w._preview_layers if l.visible} + visible = {n for _b, n, l in w._preview_layers if l.visible} assert visible == {6} assert mv.viewer.reset_view_called >= 1 # Preview surfaces load flat (no scalar coloring) for shape comparison. @@ -218,8 +236,8 @@ def test_spinbox_scrubs_visibility(self, qapp, mock_experiment_manager, tmp_path w._refresh_accept_components() w._preview_iterations() - w._component_steps["IMM"].value = 1 # emits changed -> _on_step_changed - visible = {n for _c, n, l in w._preview_layers if l.visible} + w._surface_steps["t_IMM"].value = 1 # emits changed -> _on_step_changed + visible = {n for _b, n, l in w._preview_layers if l.visible} assert visible == {1} def test_clear_preview_removes_layers(self, qapp, mock_experiment_manager, tmp_path): From e1fd32f9341c0c1f56059111926576c94c705bdf Mon Sep 17 00:00:00 2001 From: Hemanth Kapa Date: Mon, 6 Jul 2026 10:54:12 -0700 Subject: [PATCH 5/7] Load refinement previews on demand so large jobs stay responsive. Avoid parsing every iteration for every surface up front; cache meshes for scrubbing and use single-surface mode when surface count exceeds the threshold. Co-authored-by: Cursor --- .../jobs/refinement_tab.py | 261 ++++++++++++++---- .../plugins/mesh_viewer.py | 54 +++- tests/test_refinement_tab.py | 133 +++++++-- 3 files changed, 357 insertions(+), 91 deletions(-) diff --git a/src/surface_morphometrics_gui/jobs/refinement_tab.py b/src/surface_morphometrics_gui/jobs/refinement_tab.py index 78e93d2..51981b4 100644 --- a/src/surface_morphometrics_gui/jobs/refinement_tab.py +++ b/src/surface_morphometrics_gui/jobs/refinement_tab.py @@ -3,12 +3,14 @@ import re import subprocess import threading +from collections import OrderedDict from pathlib import Path from magicgui import widgets from qtpy.QtCore import Qt, QTimer from ruamel.yaml import YAML from qtpy.QtWidgets import ( + QComboBox, QLabel, QMessageBox, QPushButton, @@ -39,6 +41,13 @@ '*_profile_evolution.png', ] +# Above this many surfaces, previewing one napari layer per surface is too heavy, +# so the tab switches to single-surface mode: a dropdown picks one basename and +# only that surface's current iteration is shown. +PREVIEW_ALL_THRESHOLD = 8 +# Cap on cached parsed meshes so scrubbing back is instant without unbounded RAM. +PREVIEW_CACHE_MAX = 32 + class RefinementWidget(QWidget): """Optional density-guided mesh refinement tab. @@ -66,9 +75,18 @@ def __init__(self, experiment_manager, mesh_viewer=None): # Optional MeshViewer used to preview refined iterations in napari before # accepting one. None in headless/test paths — preview is then disabled. self.mesh_viewer = mesh_viewer - # napari layers we created for the current preview: list of - # (basename, iter_n, layer) so visibility/clear needn't re-parse names. - self._preview_layers = [] + # Lazy preview state. Rather than loading every iteration of every + # surface up front, we scan the work dir for paths only, then load the + # current spinbox iteration on demand and cache parsed meshes. + # _preview_catalog: basename -> {iter_n: Path} (filesystem scan) + # _preview_mesh_cache: (basename, iter_n) -> mesh_tuple (LRU-bounded) + # _preview_layers: basename -> napari layer (<=1 per surface, + # or exactly one total in large single-surface mode) + # _preview_active_basename: the surface shown in large mode (else None) + self._preview_catalog = {} + self._preview_mesh_cache = OrderedDict() + self._preview_layers = {} + self._preview_active_basename = None self.is_running = False main_layout = QVBoxLayout() @@ -178,9 +196,19 @@ def __init__(self, experiment_manager, mesh_viewer=None): # is the one Accept promotes. Only available when a MeshViewer was wired in. if self.mesh_viewer is not None: inner_layout.addWidget(QLabel( - "Preview loads each iteration (plus iter0, the original) as napari\n" - "surfaces per row above; scrub a spinbox — the shown iteration is\n" - "the one Accept promotes for that surface.")) + "Preview loads each surface's current iteration as a napari surface;\n" + "scrub a spinbox — the shown iteration is the one Accept promotes for\n" + "that surface. With many surfaces, pick one below to preview at a time.")) + # Large-mode surface picker: only one layer is shown at a time when + # there are more than PREVIEW_ALL_THRESHOLD surfaces. Hidden otherwise. + self.preview_combo_label = QLabel("Preview surface:") + self.preview_combo_label.setVisible(False) + inner_layout.addWidget(self.preview_combo_label) + self.preview_surface_combo = QComboBox() + self.preview_surface_combo.setVisible(False) + self.preview_surface_combo.currentTextChanged.connect( + self._on_preview_surface_changed) + inner_layout.addWidget(self.preview_surface_combo) self.preview_btn = QPushButton('Preview Iterations') self.preview_btn.clicked.connect(self._preview_iterations) inner_layout.addWidget(self.preview_btn) @@ -190,6 +218,8 @@ def __init__(self, experiment_manager, mesh_viewer=None): else: self.preview_btn = None self.clear_preview_btn = None + self.preview_surface_combo = None + self.preview_combo_label = None self.accept_btn = QPushButton('Accept Iterations') self.accept_btn.clicked.connect(self._accept_iteration) @@ -462,6 +492,7 @@ def _refresh_accept_components(self): self.accept_btn.setEnabled(False) if self.preview_btn is not None: self.preview_btn.setEnabled(False) + self._populate_preview_combo() return for basename, iters in found.items(): @@ -479,13 +510,54 @@ def _refresh_accept_components(self): self.accept_btn.setEnabled(not self.is_running) if self.preview_btn is not None: self.preview_btn.setEnabled(not self.is_running) + self._populate_preview_combo() # ----- Preview iterations in napari ----- + def _populate_preview_combo(self): + """Refill the large-mode surface picker and show it only when needed.""" + if self.preview_surface_combo is None: + return + large = len(self._surface_steps) > PREVIEW_ALL_THRESHOLD + # Block signals so refilling doesn't fire a spurious surface swap. + self.preview_surface_combo.blockSignals(True) + self.preview_surface_combo.clear() + self.preview_surface_combo.addItems(list(self._surface_steps)) + self.preview_surface_combo.blockSignals(False) + self.preview_surface_combo.setVisible(large) + if self.preview_combo_label is not None: + self.preview_combo_label.setVisible(large) + + def _build_preview_catalog(self, work_dir): + """Scan the work dir for each surface's iteration files (paths only). + + Returns ``{basename: {iter_n: Path}}`` including iter0 (the original + ``{basename}.surface.vtp``) when present. No VTK parsing happens here. + """ + pat = re.compile(r'^(?P.+)_refined_iter(?P\d+)\.surface\.vtp$') + catalog = {} + for basename in self._surface_steps: + iters = {} + for p in sorted(work_dir.glob(f'{basename}_refined_iter*.surface.vtp')): + m = pat.match(p.name) + if not m or m.group('base') != basename: + continue + iters[int(m.group('n'))] = p + orig = work_dir / f'{basename}.surface.vtp' + if orig.exists(): + iters[0] = orig + if iters: + catalog[basename] = iters + return catalog + def _preview_iterations(self): - """Load every refined iteration (plus iter0 = the original surface) into - napari as surface layers, showing only the spinbox-selected iteration per - surface basename. Each accept spinbox scrubs its own layers via visibility.""" + """Preview the current iteration of each surface as a napari layer. + + Small datasets (<= PREVIEW_ALL_THRESHOLD surfaces) get one layer per + surface, each showing its spinbox iteration. Larger datasets show a + single surface at a time, chosen by the picker combo. Only the shown + iterations are parsed; scrubbing swaps layer data from the mesh cache. + """ if self.mesh_viewer is None or not self._surface_steps: return @@ -498,73 +570,154 @@ def _preview_iterations(self): QMessageBox.warning(self, "Preview Failed", f"Could not resolve work dir: {e}") return - pat = re.compile(r'^(?P.+)_refined_iter(?P\d+)\.surface\.vtp$') - loaded = 0 - for basename in self._surface_steps: - files = [] - for p in sorted(work_dir.glob(f'{basename}_refined_iter*.surface.vtp')): - m = pat.match(p.name) - if not m: - continue - files.append((int(m.group('n')), p)) - orig = work_dir / f'{basename}.surface.vtp' - if orig.exists(): - files.append((0, orig)) - - for n, path in files: - name = f'refine-preview:{basename}:iter{n}' - layer = self._add_preview_layer(str(path), name) - if layer is not None: - self._preview_layers.append((basename, n, layer)) - loaded += 1 - - if not loaded: + self._preview_catalog = self._build_preview_catalog(work_dir) + if not self._preview_catalog: QMessageBox.information( self, "Nothing to Preview", "No refined iteration surfaces were found for the current surfaces.") return - for basename in self._surface_steps: - self._on_step_changed(basename) + if len(self._surface_steps) > PREVIEW_ALL_THRESHOLD: + # Single-surface mode: load only the picked (or first) surface. + basename = None + if self.preview_surface_combo is not None: + basename = self.preview_surface_combo.currentText() or None + if basename not in self._preview_catalog: + basename = next(iter(self._preview_catalog)) + self._preview_active_basename = basename + self._ensure_preview_layer(basename) + else: + self._preview_active_basename = None + for basename in self._surface_steps: + if basename in self._preview_catalog: + self._ensure_preview_layer(basename) + self.mesh_viewer.viewer.reset_view() - def _add_preview_layer(self, path, name): + def _current_iter(self, basename, iters): + """The spinbox iteration for a surface, clamped to available files.""" + sb = self._surface_steps.get(basename) + n = sb.value if sb is not None else max(iters) + if n in iters: + return n + return min(iters, key=lambda k: abs(k - n)) + + def _get_preview_mesh(self, basename, n): + """Return the parsed mesh tuple for ``(basename, n)``, using the cache. + + On a miss, reads the file via the viewer's ``read_mesh_tuple`` and stores + it, evicting the oldest entry once the cache exceeds PREVIEW_CACHE_MAX. + """ + if self.mesh_viewer is None: + return None + key = (basename, n) + cached = self._preview_mesh_cache.get(key) + if cached is not None: + self._preview_mesh_cache.move_to_end(key) + return cached + path = self._preview_catalog.get(basename, {}).get(n) + if path is None: + return None + mesh_tuple = self.mesh_viewer.read_mesh_tuple(str(path)) + if mesh_tuple is None: + return None + self._preview_mesh_cache[key] = mesh_tuple + while len(self._preview_mesh_cache) > PREVIEW_CACHE_MAX: + self._preview_mesh_cache.popitem(last=False) + return mesh_tuple + + def _ensure_preview_layer(self, basename): + """Create the surface's preview layer if missing, else update its data.""" + if self.mesh_viewer is None: + return + iters = self._preview_catalog.get(basename) + if not iters: + return + n = self._current_iter(basename, iters) + layer = self._preview_layers.get(basename) + if layer is None: + path = iters[n] + name = f'refine-preview:{basename}:iter{n}' + layer = self._load_preview_layer(str(path), name) + if layer is not None: + self._preview_layers[basename] = layer + else: + self._apply_iter_to_layer(basename, layer, n) + + def _load_preview_layer(self, path, name): """Load a .vtp as a flat gray preview surface and return the layer. Loaded flat (no per-vertex scalar coloring) so previews show shape, not - the noisy scalar arrays some refined surfaces carry.""" + the noisy scalar arrays some refined surfaces carry. ``reset_view`` is + skipped so preview resets the camera once, after all layers are loaded.""" try: - return self.mesh_viewer._load_mesh_file(path, name=name, flat=True) + return self.mesh_viewer._load_mesh_file( + path, name=name, flat=True, reset_view=False) except Exception as e: print(f"[RefinementWidget] Failed to preview {path}: {e}") return None + def _apply_iter_to_layer(self, basename, layer, n): + """Swap an existing preview layer's geometry to iteration ``n``.""" + mesh_tuple = self._get_preview_mesh(basename, n) + if mesh_tuple is None: + return + try: + layer.data = mesh_tuple + layer.name = f'refine-preview:{basename}:iter{n}' + except Exception as e: + print(f"[RefinementWidget] Failed to update preview {basename}: {e}") + + def _on_preview_surface_changed(self, basename): + """Combo callback: swap the single previewed surface in large mode.""" + if not basename or not self._preview_catalog: + return + if len(self._surface_steps) <= PREVIEW_ALL_THRESHOLD: + return + self._set_active_preview_surface(basename) + + def _set_active_preview_surface(self, basename): + """Show ``basename`` as the sole preview layer (single-surface mode).""" + if self.mesh_viewer is None or basename not in self._preview_catalog: + return + self._remove_preview_layers() + self._preview_active_basename = basename + self._ensure_preview_layer(basename) + self.mesh_viewer.viewer.reset_view() + def _on_step_changed(self, basename): - """Show only the selected iteration's layer(s) for this surface.""" - sb = self._surface_steps.get(basename) - if sb is None or not self._preview_layers: + """Update the surface's layer data to its newly selected iteration.""" + if self.mesh_viewer is None or not self._preview_catalog: + return + # In single-surface mode only the active surface has a layer. + if (self._preview_active_basename is not None + and basename != self._preview_active_basename): return - target = sb.value - for base, n, layer in self._preview_layers: - if base == basename: + layer = self._preview_layers.get(basename) + iters = self._preview_catalog.get(basename) + if layer is None or not iters: + return + n = self._current_iter(basename, iters) + self._apply_iter_to_layer(basename, layer, n) + + def _remove_preview_layers(self): + """Remove the napari layers we created, keeping catalog/cache intact.""" + if self.mesh_viewer is not None: + layers = self.mesh_viewer.viewer.layers + for layer in self._preview_layers.values(): try: - layer.visible = (n == target) + if layer in layers: + layers.remove(layer) except Exception: pass + self._preview_layers = {} def _clear_preview(self): - """Remove all preview layers we created from the napari viewer.""" - if self.mesh_viewer is None: - self._preview_layers = [] - return - layers = self.mesh_viewer.viewer.layers - for _comp, _n, layer in self._preview_layers: - try: - if layer in layers: - layers.remove(layer) - except Exception: - pass - self._preview_layers = [] + """Remove preview layers and drop the catalog/cache/active selection.""" + self._remove_preview_layers() + self._preview_catalog = {} + self._preview_mesh_cache.clear() + self._preview_active_basename = None def _accept_iteration(self): if self.is_running: diff --git a/src/surface_morphometrics_gui/plugins/mesh_viewer.py b/src/surface_morphometrics_gui/plugins/mesh_viewer.py index ea00ba2..866d84b 100644 --- a/src/surface_morphometrics_gui/plugins/mesh_viewer.py +++ b/src/surface_morphometrics_gui/plugins/mesh_viewer.py @@ -191,14 +191,11 @@ def _on_load_mesh_clicked(self): self._load_mesh_file(filepath) - def _load_mesh_file(self, filepath, name=None, flat=False): - """Load a mesh file using VTK and add it to napari. + def read_mesh_tuple(self, filepath): + """Read a mesh file with VTK and return ``(vertices, faces, values)``. - flat=True loads the mesh as a matte gray surface for shape comparison: - it skips the per-vertex scalar auto-coloring (marks the layer already - initialized and omits ``source_vtp_path``), so the noisy scalar arrays - some VTPs carry don't paint the surface. Ambient occlusion still applies, - giving depth. Returns the created layer (or None on failure). + Pure I/O: no napari calls, so it can be used off the layer-creation path + (e.g. to swap a preview layer's data). Returns None on failure. """ ext = os.path.splitext(filepath)[1].lower() @@ -213,7 +210,7 @@ def _load_mesh_file(self, filepath, name=None, flat=False): reader = vtk.vtkOBJReader() else: print(f"Unsupported file format: {ext}") - return + return None reader.SetFileName(filepath) reader.Update() @@ -221,7 +218,7 @@ def _load_mesh_file(self, filepath, name=None, flat=False): if polydata is None or polydata.GetNumberOfPoints() == 0: print(f"Failed to load mesh from {filepath}") - return + return None # Extract vertices vtk_points = polydata.GetPoints() @@ -231,7 +228,7 @@ def _load_mesh_file(self, filepath, name=None, flat=False): vtk_cells = polydata.GetPolys() if vtk_cells is None or vtk_cells.GetNumberOfCells() == 0: print(f"No polygon data in {filepath}") - return + return None cell_array = numpy_support.vtk_to_numpy(vtk_cells.GetData()) # VTK cell array format: [n_verts, v0, v1, v2, n_verts, v0, v1, v2, ...] @@ -242,7 +239,25 @@ def _load_mesh_file(self, filepath, name=None, flat=False): # Default scalar values (ones so AO can attenuate them) values = np.ones(len(vertices)) - mesh_tuple = (vertices, faces, values) + return (vertices, faces, values) + + def _load_mesh_file(self, filepath, name=None, flat=False, reset_view=True): + """Load a mesh file using VTK and add it to napari. + + flat=True loads the mesh as a matte gray surface for shape comparison: + it skips the per-vertex scalar auto-coloring (marks the layer already + initialized and omits ``source_vtp_path``), so the noisy scalar arrays + some VTPs carry don't paint the surface. Ambient occlusion still applies, + giving depth. Returns the created layer (or None on failure). + + reset_view=False skips the camera reset — callers that load several + layers at once (e.g. preview) can reset the view a single time instead. + """ + mesh_tuple = self.read_mesh_tuple(filepath) + if mesh_tuple is None: + return None + + ext = os.path.splitext(filepath)[1].lower() if name is None: name = os.path.splitext(os.path.basename(filepath))[0] @@ -262,9 +277,24 @@ def _load_mesh_file(self, filepath, name=None, flat=False): self.viewer.dims.ndisplay = 3 layer = self.viewer.add_surface(mesh_tuple, **add_kwargs) - self.viewer.reset_view() + if reset_view: + self.viewer.reset_view() return layer + def update_surface_layer(self, layer, filepath, flat=True): + """Swap an existing surface layer's geometry to another mesh file. + + Reads ``filepath`` via :meth:`read_mesh_tuple` and assigns the resulting + ``(vertices, faces, values)`` to ``layer.data`` so the on-screen surface + updates without creating a new layer (used to scrub preview iterations). + Returns the mesh tuple on success, or None on failure. + """ + mesh_tuple = self.read_mesh_tuple(filepath) + if mesh_tuple is None: + return None + layer.data = mesh_tuple + return mesh_tuple + def _is_vtp_surface_layer(self, layer): """Checks if a layer is a Surface derived from a VTP file.""" if not isinstance(layer, Surface): diff --git a/tests/test_refinement_tab.py b/tests/test_refinement_tab.py index 02d5dfa..fcb04ad 100644 --- a/tests/test_refinement_tab.py +++ b/tests/test_refinement_tab.py @@ -157,6 +157,7 @@ def __init__(self, path): self.path = path self.name = path self.visible = True + self.data = None class _FakeLayerViewer: @@ -169,19 +170,37 @@ def reset_view(self): class _FakeMeshViewer: - """Minimal stand-in: _load_mesh_file appends a surface layer to the viewer.""" + """Minimal stand-in for MeshViewer used by the lazy preview path. + + ``_load_mesh_file`` appends a surface layer to the viewer (layer creation). + ``read_mesh_tuple`` records disk reads separately so tests can assert the + mesh cache prevents re-reads while scrubbing. + """ def __init__(self): self.viewer = _FakeLayerViewer() - self.loaded = [] + self.loaded = [] # _load_mesh_file calls (layer creation) + self.reads = [] # read_mesh_tuple calls (disk parses used for scrubbing) + + def read_mesh_tuple(self, path): + self.reads.append(path) + # Distinct tuple per path so layer.data swaps are observable. + return ("verts", "faces", path) - def _load_mesh_file(self, path, name=None, flat=False): - self.loaded.append((path, name, flat)) + def _load_mesh_file(self, path, name=None, flat=False, reset_view=True): + self.loaded.append((path, name, flat, reset_view)) layer = _FakeLayer(path) if name is not None: layer.name = name + # Layer creation carries its own data; not counted as a scrub read. + layer.data = ("loaded", path) self.viewer.layers.append(layer) return layer + def update_surface_layer(self, layer, filepath, flat=True): + mesh_tuple = self.read_mesh_tuple(filepath) + layer.data = mesh_tuple + return mesh_tuple + @pytest.mark.gui class TestRefinementPreview: @@ -201,46 +220,75 @@ def _setup_refined(self, mock_experiment_manager, tmp_path, files): (work_dir / name).write_text("") return work_dir - def test_preview_builds_layers_including_iter0(self, qapp, mock_experiment_manager, tmp_path): + def test_preview_loads_only_current_iteration(self, qapp, mock_experiment_manager, tmp_path): mv = _FakeMeshViewer() w = self._make_widget(mock_experiment_manager, mv) self._setup_refined(mock_experiment_manager, tmp_path, [ "t_IMM.surface.vtp", # iter0 = original "t_IMM_refined_iter1.surface.vtp", "t_IMM_refined_iter6.surface.vtp", + "t_OMM.surface.vtp", + "t_OMM_refined_iter1.surface.vtp", + "t_OMM_refined_iter5.surface.vtp", ]) w._refresh_accept_components() w._preview_iterations() - names = {l.name for _b, _n, l in w._preview_layers} - assert names == { - "refine-preview:t_IMM:iter0", - "refine-preview:t_IMM:iter1", - "refine-preview:t_IMM:iter6", - } - # Default spinbox value (final iter) is the only visible layer. - visible = {n for _b, n, l in w._preview_layers if l.visible} - assert visible == {6} - assert mv.viewer.reset_view_called >= 1 - # Preview surfaces load flat (no scalar coloring) for shape comparison. - assert all(flat for _p, _n, flat in mv.loaded) - - def test_spinbox_scrubs_visibility(self, qapp, mock_experiment_manager, tmp_path): + # One layer per surface, each at its current (default = final) iteration. + # No eager loading of iter0/iter1 layers. + assert set(w._preview_layers) == {"t_IMM", "t_OMM"} + assert w._preview_layers["t_IMM"].name == "refine-preview:t_IMM:iter6" + assert w._preview_layers["t_OMM"].name == "refine-preview:t_OMM:iter5" + assert len(mv.viewer.layers) == 2 + # Only the current iteration was read from disk (via _load_mesh_file). + assert len(mv.loaded) == 2 + # Preview surfaces load flat and skip the per-load camera reset. + assert all(flat for _p, _n, flat, _rv in mv.loaded) + assert all(rv is False for _p, _n, _f, rv in mv.loaded) + # Single camera reset for the whole preview, not one per layer. + assert mv.viewer.reset_view_called == 1 + # Small dataset: the large-mode picker stays hidden. + assert w.preview_surface_combo.isHidden() + + def test_scrub_updates_layer_data_without_new_layer(self, qapp, mock_experiment_manager, tmp_path): mv = _FakeMeshViewer() w = self._make_widget(mock_experiment_manager, mv) self._setup_refined(mock_experiment_manager, tmp_path, [ - "t_IMM.surface.vtp", "t_IMM_refined_iter1.surface.vtp", "t_IMM_refined_iter6.surface.vtp", ]) w._refresh_accept_components() w._preview_iterations() + layer = w._preview_layers["t_IMM"] + assert len(mv.viewer.layers) == 1 w._surface_steps["t_IMM"].value = 1 # emits changed -> _on_step_changed - visible = {n for _b, n, l in w._preview_layers if l.visible} - assert visible == {1} - def test_clear_preview_removes_layers(self, qapp, mock_experiment_manager, tmp_path): + # No new layer created; the same layer's data/name reflects iter1. + assert len(mv.viewer.layers) == 1 + assert w._preview_layers["t_IMM"] is layer + assert layer.name == "refine-preview:t_IMM:iter1" + iter1_path = str(w._preview_catalog["t_IMM"][1]) + assert layer.data == ("verts", "faces", iter1_path) + + def test_scrub_cache_hit_avoids_reread(self, qapp, mock_experiment_manager, tmp_path): + mv = _FakeMeshViewer() + w = self._make_widget(mock_experiment_manager, mv) + self._setup_refined(mock_experiment_manager, tmp_path, [ + "t_IMM_refined_iter1.surface.vtp", + "t_IMM_refined_iter6.surface.vtp", + ]) + w._refresh_accept_components() + w._preview_iterations() + + iter1_path = str(w._preview_catalog["t_IMM"][1]) + w._surface_steps["t_IMM"].value = 1 # first scrub to iter1 -> disk read + assert mv.reads.count(iter1_path) == 1 + w._surface_steps["t_IMM"].value = 6 # scrub away + w._surface_steps["t_IMM"].value = 1 # scrub back -> served from cache + assert mv.reads.count(iter1_path) == 1 + + def test_clear_preview_removes_layers_and_cache(self, qapp, mock_experiment_manager, tmp_path): mv = _FakeMeshViewer() w = self._make_widget(mock_experiment_manager, mv) self._setup_refined(mock_experiment_manager, tmp_path, [ @@ -250,7 +298,9 @@ def test_clear_preview_removes_layers(self, qapp, mock_experiment_manager, tmp_p w._preview_iterations() assert mv.viewer.layers w._clear_preview() - assert w._preview_layers == [] + assert w._preview_layers == {} + assert w._preview_catalog == {} + assert len(w._preview_mesh_cache) == 0 assert mv.viewer.layers == [] def test_refresh_clears_stale_preview(self, qapp, mock_experiment_manager, tmp_path): @@ -263,10 +313,43 @@ def test_refresh_clears_stale_preview(self, qapp, mock_experiment_manager, tmp_p w._preview_iterations() assert w._preview_layers w._refresh_accept_components() - assert w._preview_layers == [] + assert w._preview_layers == {} + assert w._preview_catalog == {} assert mv.viewer.layers == [] + def test_large_mode_single_layer_and_combo_swap(self, qapp, mock_experiment_manager, tmp_path): + mv = _FakeMeshViewer() + w = self._make_widget(mock_experiment_manager, mv) + # 9 surfaces > PREVIEW_ALL_THRESHOLD (8) triggers single-surface mode. + files = [] + for i in range(9): + files.append(f"t{i}_IMM_refined_iter1.surface.vtp") + files.append(f"t{i}_IMM_refined_iter6.surface.vtp") + self._setup_refined(mock_experiment_manager, tmp_path, files) + w._refresh_accept_components() + + assert len(w._surface_steps) == 9 + # Large mode shows the surface picker. + assert not w.preview_surface_combo.isHidden() + assert w.preview_surface_combo.count() == 9 + + w._preview_iterations() + # Only one layer exists even though there are 9 surfaces. + assert len(mv.viewer.layers) == 1 + assert len(w._preview_layers) == 1 + first = w.preview_surface_combo.itemText(0) + assert set(w._preview_layers) == {first} + assert w._preview_active_basename == first + + # Switching the combo swaps to the other surface, still one layer. + other = w.preview_surface_combo.itemText(1) + w.preview_surface_combo.setCurrentText(other) + assert len(mv.viewer.layers) == 1 + assert set(w._preview_layers) == {other} + assert w._preview_active_basename == other + def test_no_viewer_disables_preview(self, qapp, mock_experiment_manager): w = self._make_widget(mock_experiment_manager, None) assert w.preview_btn is None assert w.clear_preview_btn is None + assert w.preview_surface_combo is None From 0082c03a1287f333866221247cd72a368831e7d4 Mon Sep 17 00:00:00 2001 From: Hemanth Kapa Date: Mon, 6 Jul 2026 11:02:58 -0700 Subject: [PATCH 6/7] Fix refinement refresh after resume by honoring saved work_dir. Resume now syncs experiment UI from config and discovery uses the config work_dir so Accept Iteration surfaces appear when refinement files exist. Co-authored-by: Cursor --- .../experiment_manager.py | 45 ++++++------ .../jobs/refinement_tab.py | 68 +++++++++++++++---- .../utils/script_resolver.py | 3 +- tests/test_refinement_tab.py | 27 ++++++++ 4 files changed, 104 insertions(+), 39 deletions(-) diff --git a/src/surface_morphometrics_gui/experiment_manager.py b/src/surface_morphometrics_gui/experiment_manager.py index 5da6312..949521a 100644 --- a/src/surface_morphometrics_gui/experiment_manager.py +++ b/src/surface_morphometrics_gui/experiment_manager.py @@ -422,6 +422,23 @@ def _on_experiment_selected(self): # User is typing a new name — just update button text self.submit_button.setText('Start New Experiment') + def _apply_config_to_ui(self, existing_config, config_path=None): + """Push a loaded experiment config into the manager widgets.""" + if get_seg_dir(existing_config): + self.data_dir.value = get_seg_dir(existing_config) + self.config_template.changed.disconnect(self._handle_config_template_selection) + try: + if 'config_template' in existing_config: + self.config_template.value = existing_config['config_template'] + elif config_path is not None: + self.config_template.value = str(config_path) + finally: + self.config_template.changed.connect(self._handle_config_template_selection) + if 'cores' in existing_config: + self.cores_input.setValue(existing_config['cores']) + if 'segmentation_values' in existing_config: + self.segmentation_container._set_values(existing_config['segmentation_values']) + def _load_existing_experiment_config(self): """Load configuration from an existing experiment""" @@ -457,34 +474,12 @@ def _load_existing_experiment_config(self): # the flat exp_dir for a raw CLI project). self.current_config['work_dir'] = cli_work_dir(resolve_work_dir(exp_dir)) - # Block config_template.changed while restoring UI so that - # _handle_config_template_selection does not re-load the - # original template file and overwrite current_config / data_dir. - self.config_template.changed.disconnect(self._handle_config_template_selection) - try: - # Update UI with loaded config values - if get_seg_dir(existing_config): - self.data_dir.value = get_seg_dir(existing_config) - # Load the original config template if available - if 'config_template' in existing_config: - self.config_template.value = existing_config['config_template'] - else: - self.config_template.value = str(config_path) - finally: - self.config_template.changed.connect(self._handle_config_template_selection) - # Set cores if available - if 'cores' in existing_config: - - self.cores_input.setValue(existing_config['cores']) - # Load segmentation values if available - if 'segmentation_values' in existing_config: - - self.segmentation_container._set_values(existing_config['segmentation_values']) + self._apply_config_to_ui(existing_config, config_path=config_path) # Emit signal that config was loaded - this will update job tabs self.config_loaded.emit() except Exception as e: - pass + print(f'[Resume] Failed to load experiment config: {e}') def _check_start_button_state(self): """Enable start button only when all required fields are filled""" @@ -690,6 +685,8 @@ def _resume_experiment(self): # so the config the tabs read matches where the files really are. self.current_config['work_dir'] = cli_work_dir(resolve_work_dir(exp_dir)) + self._apply_config_to_ui(self.current_config, config_path=config_path) + # Emit signal that config was loaded - this will update job tabs self.config_loaded.emit() diff --git a/src/surface_morphometrics_gui/jobs/refinement_tab.py b/src/surface_morphometrics_gui/jobs/refinement_tab.py index 51981b4..6959546 100644 --- a/src/surface_morphometrics_gui/jobs/refinement_tab.py +++ b/src/surface_morphometrics_gui/jobs/refinement_tab.py @@ -263,12 +263,51 @@ def _on_config_loaded(self): print(f"[RefinementWidget] Error in _on_config_loaded: {e}") def _config_path(self): - exp_name = self.experiment_manager.experiment_name.currentText() + exp_name = self.experiment_manager.experiment_name.currentText().strip() exp_dir = Path(self.experiment_manager.work_dir.value) / exp_name preferred = exp_dir / f"{exp_name}_config.yml" fallback = exp_dir / 'config.yml' return (preferred if preferred.exists() else fallback), exp_dir + def _resolve_work_dir(self): + """Directory where refinement outputs live for the loaded experiment. + + On resume the saved config's ``work_dir`` is authoritative (set by + ExperimentManager when the experiment is loaded). Re-deriving from the + GUI parent path alone can miss files when layout or paths differ. + """ + candidates = [] + config = self.experiment_manager.current_config or {} + cfg_work = config.get('work_dir') + if cfg_work: + candidates.append(Path(str(cfg_work).rstrip(os.sep))) + try: + _, exp_dir = self._config_path() + candidates.append(resolve_work_dir(exp_dir)) + candidates.append(exp_dir) + except Exception as e: + print(f"[RefinementWidget] Could not resolve experiment dir: {e}") + + seen = set() + unique = [] + for d in candidates: + if d is None: + continue + p = Path(d) + key = str(p.resolve()) if p.exists() else str(p) + if key in seen: + continue + seen.add(key) + unique.append(p) + + for p in unique: + if p.is_dir() and list(p.glob('*_refined_iter*.surface.vtp')): + return p + for p in unique: + if p.is_dir(): + return p + return None + def _radius_hit(self): config = self.experiment_manager.current_config or {} return config.get('curvature_measurements', {}).get('radius_hit', 9) @@ -338,7 +377,11 @@ def _run_refinement(self): return config_path, exp_dir = self._config_path() - work_dir = resolve_work_dir(exp_dir) + work_dir = self._resolve_work_dir() + if work_dir is None: + QMessageBox.warning(self, "No Work Directory", + "Could not resolve the experiment output directory.") + return radius_hit = self._radius_hit() if not list(work_dir.glob(f'*.AVV_rh{radius_hit}.gt')): QMessageBox.warning( @@ -479,12 +522,7 @@ def _refresh_accept_components(self): self.accept_container.clear() self._surface_steps = {} - try: - _, exp_dir = self._config_path() - work_dir = resolve_work_dir(exp_dir) - except Exception: - work_dir = None - + work_dir = self._resolve_work_dir() found = self._discover_refined_surfaces(work_dir) if work_dir else {} if not found: self.accept_container.append(widgets.Label( @@ -563,11 +601,9 @@ def _preview_iterations(self): self._clear_preview() - try: - _, exp_dir = self._config_path() - work_dir = resolve_work_dir(exp_dir) - except Exception as e: - QMessageBox.warning(self, "Preview Failed", f"Could not resolve work dir: {e}") + work_dir = self._resolve_work_dir() + if work_dir is None: + QMessageBox.warning(self, "Preview Failed", "Could not resolve work dir.") return self._preview_catalog = self._build_preview_catalog(work_dir) @@ -737,7 +773,11 @@ def _accept_iteration(self): "No refined surfaces found. Run refinement, then Refresh.") return - work_dir = resolve_work_dir(exp_dir) + work_dir = self._resolve_work_dir() + if work_dir is None: + QMessageBox.warning(self, "No Work Directory", + "Could not resolve the experiment output directory.") + return choices = {b: sb.value for b, sb in self._surface_steps.items()} # Validate each chosen iteration exists for its surface before touching files. missing = [f"{b}: iteration {s}" for b, s in choices.items() diff --git a/src/surface_morphometrics_gui/utils/script_resolver.py b/src/surface_morphometrics_gui/utils/script_resolver.py index 9f459a3..d160082 100644 --- a/src/surface_morphometrics_gui/utils/script_resolver.py +++ b/src/surface_morphometrics_gui/utils/script_resolver.py @@ -46,7 +46,8 @@ def cli_work_dir(results_directory): # Filenames the pipeline produces at any stage — used to detect whether a # directory already holds pipeline outputs. _OUTPUT_MARKERS = ("*.surface.vtp", "*.AVV_rh*.gt", "*.AVV_rh*.vtp", - "*.AVV_rh*.csv", "*.ply", "*.xyz") + "*.AVV_rh*.csv", "*.ply", "*.xyz", + "*_refined_iter*.surface.vtp") def _has_pipeline_outputs(directory): diff --git a/tests/test_refinement_tab.py b/tests/test_refinement_tab.py index fcb04ad..6a5aa1e 100644 --- a/tests/test_refinement_tab.py +++ b/tests/test_refinement_tab.py @@ -127,6 +127,33 @@ def test_refresh_no_files_disables_accept(self, qapp, mock_experiment_manager, t assert w._surface_steps == {} assert not w.accept_btn.isEnabled() + def test_refresh_uses_saved_config_work_dir(self, qapp, mock_experiment_manager, tmp_path): + """After resume, discovery must honor config work_dir, not re-guess paths.""" + import os + w = self._make_widget(qapp, mock_experiment_manager) + exp_parent = tmp_path / "projects" + exp_parent.mkdir() + exp_dir = exp_parent / "exp" + exp_dir.mkdir() + work_dir = exp_dir / "results" + work_dir.mkdir() + for name in [ + "t_IMM_refined_iter1.surface.vtp", + "t_IMM_refined_iter6.surface.vtp", + ]: + (work_dir / name).write_text("") + (exp_dir / "exp_config.yml").write_text("work_dir: placeholder\n") + + mock_experiment_manager.work_dir.value = str(exp_parent) + mock_experiment_manager.experiment_name.currentText.return_value = "exp" + mock_experiment_manager.current_config = { + "work_dir": str(work_dir) + os.sep, + } + + w._refresh_accept_components() + assert set(w._surface_steps) == {"t_IMM"} + assert w._surface_steps["t_IMM"].value == 6 + def test_accept_worker_runs_one_call_per_surface(self, qapp, mock_experiment_manager, tmp_path): w = self._make_widget(qapp, mock_experiment_manager) work_dir = self._setup_refined(w, mock_experiment_manager, tmp_path, []) From bbe5b75510d832820e035c647f18d2b0190b862d Mon Sep 17 00:00:00 2001 From: Hemanth Kapa Date: Mon, 6 Jul 2026 11:07:23 -0700 Subject: [PATCH 7/7] Broaden refinement discovery paths after resume. Scan config work_dir (including relative results/), exp_dir, and results/; handle work_dir field pointing at the experiment folder; log scanned paths. Co-authored-by: Cursor --- .../experiment_manager.py | 2 + .../jobs/refinement_tab.py | 122 ++++++++++++------ .../utils/script_resolver.py | 42 ++++++ tests/test_experiment_manager.py | 7 + tests/test_refinement_tab.py | 33 +++++ 5 files changed, 168 insertions(+), 38 deletions(-) diff --git a/src/surface_morphometrics_gui/experiment_manager.py b/src/surface_morphometrics_gui/experiment_manager.py index 949521a..5d33b9a 100644 --- a/src/surface_morphometrics_gui/experiment_manager.py +++ b/src/surface_morphometrics_gui/experiment_manager.py @@ -862,6 +862,8 @@ def _import_cli_project(self): idx = self.experiment_name.findText(plan.exp_name) if idx >= 0: self.experiment_name.setCurrentIndex(idx) + # Ensure job tabs see the adopted config even if the index didn't change. + self._load_existing_experiment_config() if result.failed_moves: detail = "\n".join(f" {s.name}: {msg}" for s, msg in result.failed_moves[:5]) diff --git a/src/surface_morphometrics_gui/jobs/refinement_tab.py b/src/surface_morphometrics_gui/jobs/refinement_tab.py index 6959546..db5c833 100644 --- a/src/surface_morphometrics_gui/jobs/refinement_tab.py +++ b/src/surface_morphometrics_gui/jobs/refinement_tab.py @@ -26,6 +26,8 @@ REFINE_MESH, ACCEPT_REFINEMENT, resolve_work_dir, + resolve_config_work_dir, + work_dir_search_candidates, cli_work_dir, ) from ..widgets.job_status import JobStatusWidget @@ -262,51 +264,92 @@ def _on_config_loaded(self): except Exception as e: print(f"[RefinementWidget] Error in _on_config_loaded: {e}") + def _resolve_exp_dir(self): + """Experiment directory from the manager's work-dir field and name. + + The work-dir field is usually the parent of experiment folders, but + users sometimes point it directly at an experiment directory. + """ + exp_name = self.experiment_manager.experiment_name.currentText().strip() + parent = Path(str(self.experiment_manager.work_dir.value or '')) + if not exp_name: + raise ValueError("No experiment selected") + if not parent: + raise ValueError("Work directory not set") + + nested = parent / exp_name + if nested.is_dir() and ( + (nested / 'config.yml').exists() or list(nested.glob('*_config.yml')) + ): + return nested + if (parent / 'config.yml').exists() or list(parent.glob('*_config.yml')): + return parent + if parent.name == exp_name and parent.is_dir(): + return parent + return nested + def _config_path(self): exp_name = self.experiment_manager.experiment_name.currentText().strip() - exp_dir = Path(self.experiment_manager.work_dir.value) / exp_name + exp_dir = self._resolve_exp_dir() preferred = exp_dir / f"{exp_name}_config.yml" fallback = exp_dir / 'config.yml' return (preferred if preferred.exists() else fallback), exp_dir - def _resolve_work_dir(self): - """Directory where refinement outputs live for the loaded experiment. - - On resume the saved config's ``work_dir`` is authoritative (set by - ExperimentManager when the experiment is loaded). Re-deriving from the - GUI parent path alone can miss files when layout or paths differ. - """ - candidates = [] + def _iter_work_dir_candidates(self): + """Yield directories that may contain refinement iteration surfaces.""" config = self.experiment_manager.current_config or {} - cfg_work = config.get('work_dir') - if cfg_work: - candidates.append(Path(str(cfg_work).rstrip(os.sep))) try: - _, exp_dir = self._config_path() - candidates.append(resolve_work_dir(exp_dir)) - candidates.append(exp_dir) + exp_dir = self._resolve_exp_dir() except Exception as e: print(f"[RefinementWidget] Could not resolve experiment dir: {e}") + exp_dir = None + + if exp_dir is not None: + for d in work_dir_search_candidates(config, exp_dir): + yield d + + # work_dir field may already be the experiment folder (not its parent). + raw = self.experiment_manager.work_dir.value + exp_name = self.experiment_manager.experiment_name.currentText().strip() + if raw and exp_name: + raw_p = Path(str(raw)) + if raw_p.is_dir() and raw_p.name == exp_name: + for d in work_dir_search_candidates(config, raw_p): + yield d - seen = set() - unique = [] - for d in candidates: - if d is None: + def _resolve_work_dir(self): + """Best directory for refinement outputs (first candidate with iter files).""" + _found, primary = self._discover_refined_surfaces_all() + if primary is not None: + return primary + for d in self._iter_work_dir_candidates(): + if d.is_dir(): + return d + return None + + def _discover_refined_surfaces_all(self): + """Scan every plausible output dir; merge discoveries across layouts.""" + merged = {} + primary = None + searched = [] + for d in self._iter_work_dir_candidates(): + key = str(d.resolve()) if d.exists() else str(d) + if key in searched: continue - p = Path(d) - key = str(p.resolve()) if p.exists() else str(p) - if key in seen: + searched.append(key) + if not d.is_dir(): + print(f"[RefinementWidget] Skip (not a dir): {d}") continue - seen.add(key) - unique.append(p) - - for p in unique: - if p.is_dir() and list(p.glob('*_refined_iter*.surface.vtp')): - return p - for p in unique: - if p.is_dir(): - return p - return None + part = self._discover_refined_surfaces(d) + n_files = sum(len(v) for v in part.values()) + print(f"[RefinementWidget] Scan {d}: {n_files} refined iteration(s)") + if part and primary is None: + primary = d + for basename, iters in part.items(): + merged.setdefault(basename, set()).update(iters) + if not merged: + print(f"[RefinementWidget] No refined iterations under: {', '.join(searched) or '(none)'}") + return ({b: sorted(v) for b, v in sorted(merged.items())}, primary) def _radius_hit(self): config = self.experiment_manager.current_config or {} @@ -496,9 +539,12 @@ def _discover_refined_surfaces(self, work_dir): ``basename`` is typically ``{tomogram}_{component}`` (e.g. ``TE1_IMM``). Each basename gets its own spinbox so tomograms can accept different steps. """ - pat = re.compile(r'^(?P.+)_refined_iter(?P\d+)\.surface\.vtp$') + pat = re.compile( + r'^(?P.+)_refined_iter(?P\d+)\.surface\.vtp$', re.IGNORECASE) surfaces = {} - for p in work_dir.glob('*_refined_iter*.surface.vtp'): + for p in work_dir.iterdir(): + if not p.is_file(): + continue m = pat.match(p.name) if not m: continue @@ -521,9 +567,9 @@ def _refresh_accept_components(self): self._clear_preview() self.accept_container.clear() self._surface_steps = {} + self._resolved_work_dir = None - work_dir = self._resolve_work_dir() - found = self._discover_refined_surfaces(work_dir) if work_dir else {} + found, self._resolved_work_dir = self._discover_refined_surfaces_all() if not found: self.accept_container.append(widgets.Label( value='No refined iterations found. Run refinement, then Refresh.')) @@ -601,7 +647,7 @@ def _preview_iterations(self): self._clear_preview() - work_dir = self._resolve_work_dir() + work_dir = self._resolved_work_dir or self._resolve_work_dir() if work_dir is None: QMessageBox.warning(self, "Preview Failed", "Could not resolve work dir.") return @@ -773,7 +819,7 @@ def _accept_iteration(self): "No refined surfaces found. Run refinement, then Refresh.") return - work_dir = self._resolve_work_dir() + work_dir = self._resolved_work_dir or self._resolve_work_dir() if work_dir is None: QMessageBox.warning(self, "No Work Directory", "Could not resolve the experiment output directory.") diff --git a/src/surface_morphometrics_gui/utils/script_resolver.py b/src/surface_morphometrics_gui/utils/script_resolver.py index d160082..ec670e5 100644 --- a/src/surface_morphometrics_gui/utils/script_resolver.py +++ b/src/surface_morphometrics_gui/utils/script_resolver.py @@ -58,6 +58,48 @@ def _has_pipeline_outputs(directory): for pattern in _OUTPUT_MARKERS) +def resolve_config_work_dir(work_dir_value, exp_dir): + """Normalize ``work_dir`` from a config, resolving relative paths. + + Config files may store a relative ``work_dir`` (e.g. ``results/``); interpret + that relative to the experiment directory, not the process cwd. + """ + if not work_dir_value: + return None + p = Path(str(work_dir_value).strip()) + if not p.is_absolute(): + p = (Path(exp_dir) / p).resolve() + return p + + +def work_dir_search_candidates(config, exp_dir): + """Ordered unique directories that may hold pipeline outputs for *exp_dir*.""" + exp_dir = Path(exp_dir) + ordered = [] + cfg_work = (config or {}).get('work_dir') + if cfg_work: + resolved = resolve_config_work_dir(cfg_work, exp_dir) + if resolved is not None: + ordered.append(resolved) + ordered.extend([ + resolve_work_dir(exp_dir), + exp_dir, + exp_dir / 'results', + ]) + seen = set() + out = [] + for d in ordered: + if d is None: + continue + p = Path(d) + key = str(p.resolve()) if p.exists() else str(p) + if key in seen: + continue + seen.add(key) + out.append(p) + return out + + def resolve_work_dir(exp_dir): """The directory the pipeline reads from and writes to for an experiment. diff --git a/tests/test_experiment_manager.py b/tests/test_experiment_manager.py index af928fe..d2fee04 100644 --- a/tests/test_experiment_manager.py +++ b/tests/test_experiment_manager.py @@ -487,3 +487,10 @@ def test_non_output_files_do_not_count_as_flat(self, tmp_path): exp.mkdir() (exp / "exp_config.yml").write_text("x") # config alone is not an output assert resolve_work_dir(exp) == exp / "results" + + def test_resolve_config_work_dir_relative(self, tmp_path): + from utils.script_resolver import resolve_config_work_dir + exp = tmp_path / "exp" + exp.mkdir() + out = resolve_config_work_dir("results/", exp) + assert out == (exp / "results").resolve() diff --git a/tests/test_refinement_tab.py b/tests/test_refinement_tab.py index 6a5aa1e..eedcfc8 100644 --- a/tests/test_refinement_tab.py +++ b/tests/test_refinement_tab.py @@ -154,6 +154,39 @@ def test_refresh_uses_saved_config_work_dir(self, qapp, mock_experiment_manager, assert set(w._surface_steps) == {"t_IMM"} assert w._surface_steps["t_IMM"].value == 6 + def test_refresh_finds_relative_config_work_dir(self, qapp, mock_experiment_manager, tmp_path): + """Config work_dir like results/ must resolve against exp_dir, not cwd.""" + w = self._make_widget(qapp, mock_experiment_manager) + exp_parent = tmp_path / "projects" + exp_dir = exp_parent / "exp" + work_dir = exp_dir / "results" + work_dir.mkdir(parents=True) + (work_dir / "t_IMM_refined_iter6.surface.vtp").write_text("") + (exp_dir / "exp_config.yml").write_text("work_dir: results/\n") + + mock_experiment_manager.work_dir.value = str(exp_parent) + mock_experiment_manager.experiment_name.currentText.return_value = "exp" + mock_experiment_manager.current_config = {"work_dir": "results/"} + + w._refresh_accept_components() + assert "t_IMM" in w._surface_steps + + def test_refresh_when_work_dir_field_is_experiment_dir(self, qapp, mock_experiment_manager, tmp_path): + """Work-dir field may point at the experiment folder, not its parent.""" + w = self._make_widget(qapp, mock_experiment_manager) + exp_dir = tmp_path / "myexp" + work_dir = exp_dir / "results" + work_dir.mkdir(parents=True) + (work_dir / "TE1_OMM_refined_iter3.surface.vtp").write_text("") + (exp_dir / "config.yml").write_text("work_dir: results/\n") + + mock_experiment_manager.work_dir.value = str(exp_dir) + mock_experiment_manager.experiment_name.currentText.return_value = "myexp" + mock_experiment_manager.current_config = {"work_dir": "results/"} + + w._refresh_accept_components() + assert set(w._surface_steps) == {"TE1_OMM"} + def test_accept_worker_runs_one_call_per_surface(self, qapp, mock_experiment_manager, tmp_path): w = self._make_widget(qapp, mock_experiment_manager) work_dir = self._setup_refined(w, mock_experiment_manager, tmp_path, [])