feat(wellplate): save/load z coordinates in multipoint coordinate CSVs - #608
Merged
Conversation
…ates Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oordinates Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…position The test_acquisition_moves_to_per_fov_z test was asserting the final stage position after acquisition, but the controller moves the stage back to its starting position at the end. Fixed by capturing the Z position from CaptureInfo during image acquisition via the signal_new_image callback instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… dead Save/Clear machinery - Implement on_load_or_clear_coordinates_clicked() that toggles between loading and clearing - Add clear_loaded_coordinates() to reset state and update UI - Add _set_has_loaded_coordinates() to manage button text and flag state - Set flag in load_coordinates() and restore_cached_coordinates() load paths - Delete toggle_coordinate_controls() and on_save_or_clear_coordinates_clicked() dead methods - Delete call to toggle_coordinate_controls() in acquisition_is_finished() - Update button connections: btn_load → on_load_or_clear_coordinates_clicked, btn_save → save_coordinates - Update QTimer call to use new method name - Add comprehensive TDD tests for toggle behavior and dead machinery removal Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… helper Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add three new module-level functions: _objective_relative_z_mm, parfocal_adjusted_z_mm, and coordinate_rows_for_save. Rewrite save_coordinates to stamp z values per FOV, shifted for each objective by the Xeryon switcher's per-machine Z offset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address three findings from final whole-branch review: - restore_cached_coordinates now wraps the helper call + FOV registration + text/flag updates in try/except, mirroring load_coordinates. SOFTWARE_POS_LIMIT.Z_POSITIVE/Z_NEGATIVE are runtime-mutable, so a cached dataframe that validated at load time could fail validation on a later restore (mode switch away and back), raising the helper's ValueError into the on_xy_mode_changed Qt slot and aborting the process. On failure it logs, shows a warning, and leaves has_loaded_coordinates unset. - load_coordinate_regions_from_dataframe now parses/converts every region's coordinates into local dicts before calling clear_regions() and writing into region_fov_coordinates/ region_centers, so a non-numeric x/y cell raises before any mutation instead of after clearing, which used to leave the GUI with empty regions while the cache/label still claimed a loaded file. - Reworded the helper's docstring: production code no longer calls ScanCoordinates.update_fov_z_level (only tests do), so the old justification for storing centers as [x, y] lists was stale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- share the Xeryon objective->position rule via _def.xeryon_objective_position (used by both the hardware mover and the parfocal save math) - factor the duplicated FOV-registration + z-dropped warning into _register_loaded_fovs, used by both coordinate loaders - replace the helper's per-key copy loop with dict.update() - note update_fov_z_level's caller status; reuse the toggle test fixture; drop the callback-override ordering footgun in the per-FOV-z test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds per-FOV Z support to wellplate multipoint coordinate CSV save/load while preventing focus-map acquisition from mutating GUI coordinate state, and updates the Load Coordinates UI to properly toggle between “Load New Coords” and “Clear Coords”.
Changes:
- Introduces optional
z (mm)handling in wellplate and fluidics multipoint CSV loaders/savers via shared helpers. - Fixes focus-map acquisition to rewrite only a private coordinate snapshot (no GUI state mutation).
- Implements Load-button “Clear Coords” toggle and removes dead Save/Clear toggle machinery.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| software/control/widgets.py | Shared dataframe→regions loader, Z-aware save helpers, load/clear toggle, and z-stamping/parfocal adjustment logic. |
| software/control/core/multi_point_utils.py | Deep-copies per-region FOV lists when snapshotting scan coordinates. |
| software/control/core/multi_point_controller.py | Focus-map interpolation now rewrites only the acquisition snapshot (no write-back). |
| software/control/core/scan_coordinates.py | Documents update_fov_z_level expectations around mutable region centers. |
| software/control/_def.py | Adds xeryon_objective_position() helper as a single objective→switcher-position rule source. |
| software/control/objective_changer_2_pos_controller.py | Uses xeryon_objective_position() to select switcher movement target. |
| software/tests/control/test_widgets.py | Adds tests for load/clear toggle, Z load/drop/validation, parfocal math, and save/load round-trip. |
| software/tests/control/test_MultiPointController.py | Adds tests ensuring focus map doesn’t mutate GUI coords and that per-FOV z is honored. |
| software/tests/control/core/test_multi_point_utils.py | Adds snapshot isolation test for from_scan_coordinates. |
| software/docs/superpowers/specs/2026-08-11-wellplate-z-coordinates-design.md | Design spec for Z-aware save/load and associated bug fixes. |
| software/docs/superpowers/plans/2026-08-11-wellplate-z-coordinates.md | Implementation plan documenting intended behaviors and test coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+180
to
+196
| has_z = "z (mm)" in df.columns | ||
| z_dropped = False | ||
| if has_z and df["z (mm)"].isna().any(): | ||
| # Keep every region's tuples homogeneous and never let NaN reach the stage. | ||
| has_z = False | ||
| z_dropped = True | ||
|
|
||
| if has_z: | ||
| z_min = control._def.SOFTWARE_POS_LIMIT.Z_NEGATIVE | ||
| z_max = control._def.SOFTWARE_POS_LIMIT.Z_POSITIVE | ||
| out_of_range = df[(df["z (mm)"] < z_min) | (df["z (mm)"] > z_max)] | ||
| if not out_of_range.empty: | ||
| raise ValueError( | ||
| f"z (mm) values outside software limits [{z_min}, {z_max}] mm: " | ||
| f"{sorted(out_of_range['z (mm)'].unique().tolist())}" | ||
| ) | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
[Claude Code] Fixed in commit 38c8e33 - the z column is now coerced with pd.to_numeric(errors="coerce") before validation: parseable numeric strings load correctly, and unparseable cells take the existing warn-and-load-XY-only path (same as empty cells), so the raw-column TypeError can no longer occur.
Comment on lines
+9218
to
+9225
| except Exception as e: | ||
| # SOFTWARE_POS_LIMIT.Z_POSITIVE/Z_NEGATIVE are runtime-mutable, so a | ||
| # cached dataframe that validated at load time can fail validation on | ||
| # a later restore (e.g. mode switch away and back). This method is | ||
| # called from a Qt slot, so a raised exception here would propagate | ||
| # into the event loop. | ||
| self._log.error(f"Failed to restore cached coordinates: {str(e)}") | ||
| QMessageBox.warning(self, "Load Error", f"Failed to restore cached coordinates\nError: {str(e)}") |
Collaborator
Author
There was a problem hiding this comment.
[Claude Code] Intentional - has_loaded_coordinates means "a coordinates file is loaded (cache present)", and on a failed restore the cache is still present. The failure already surfaces a warning dialog, and "Clear Coords" is the affordance for discarding the bad cache. Auto-clearing it would destroy the recovery path: Z software limits are runtime-mutable, so a cached file that fails validation can validate again after the user re-widens the limits and switches modes back.
An object-dtype z column (numeric strings from hand-edited CSVs, stray text) made the raw range comparison raise TypeError. Parseable strings now load correctly; unparseable cells take the existing warn-and-load- XY-only path. Addresses Copilot review on #608. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…plan After an acquisition finished in Load Coordinates mode, reset_coordinates -> update_coordinates fell into the Select Wells branch and silently replaced the loaded plan with regions derived from whatever wells were still ticked, while the UI kept claiming the file was loaded — the next Start then acquired the wrong regions at the wrong z. Loaded plans are owned by the load/restore/clear flow; update_coordinates now leaves them untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tch shipped behavior update_fov_z_level lost its only production caller when the focus-map write-back was replaced by the acquisition snapshot; delete it and its satellite test rather than keep documented dead code. Drop a stale save/clear comment. Amend the committed spec: the post-acquisition update_coordinates wipe was fixed on this branch (no longer out of scope), and note the dead-code removal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…loaded The Load Coordinates early return was unconditional, so re-applying an acquisition YAML recorded in that mode (drag-drop or MCP) left stale regions behind a UI showing the YAML's ticked wells. Guard on has_loaded_coordinates instead: loaded plans stay protected, and with no plan the ticked wells derive regions exactly as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wellplate multipoint coordinate CSVs gain an optional
z (mm)column, so a saved scan plan controls the Z each FOV is acquired at — matching the format the flexible widget and the acquisition's own top-levelcoordinates.csvalready use (a previous acquisition's plan is directly re-loadable, Z included).Save Coordinates): every FOV row is stamped with the current stage Z. Per-objective files are parfocal-corrected using the existing Xeryon 2-position switcher config (XERYON_OBJECTIVE_SWITCHER_POS_1/POS_2+POS_2_OFFSET_MM); the objective→position rule is single-sourced in_def.xeryon_objective_position(), now used by both the hardware mover and the save math. Machines without the switcher save identical Z in every file (previous behavior). No new config.Load Coordinatesmode + fluidics widget): one shared dataframe→regions helper builds(x, y, z)FOV tuples the acquisition worker already honors (move_to_coordinate). CSVs without a z column load exactly as before. The z column is coerced to numeric up front: parseable strings load correctly; empty or unparseable cells drop the column with a warning (XY-only, regions stay tuple-length homogeneous); out-of-range z rejects the file. All parsing/validation happens before any mutation, so a bad file never half-destroys existing regions.Load New Coordsbutton toggles toClear Coordswhile a file is loaded; the dead Save↔Clear button-morphing machinery (has_loaded_coordinateswas never set) is removed.Bug fixes folded in
ScanPositionInformation.from_scan_coordinatesdeep-copies the per-region FOV lists (a true snapshot) and the focus-map write-back is removed — interpolated Z used to leak intoScanCoordinates, silently carry into later runs, and crash a subsequent save on 3-tuples.update_fov_z_level, left with no callers, is deleted.update_coordinates()can no longer regenerate over a loaded plan. Previously, the post-acquisition reset fell into the Select-Wells branch and silently replaced a loaded plan with whatever wells were ticked — the next Start then acquired the wrong regions at the wrong Z while the UI still claimed the file was loaded. Found by hands-on smoke testing on this branch; loaded plans are now owned exclusively by the Load / Clear / mode-switch flow.AttributeError: tuple centers) — loaders store mutable[x, y]list centers; the crashing write-back path is gone entirely.restore_cached_coordinatescalled the nonexistentregister_fov_to_image; it now shares the loader helper and is hardened with try/except (Z limits are runtime-mutable, so a cached file can fail re-validation; an unhandled slot exception aborts PyQt5).Test plan
black --checkclean.CaptureInfo.position.z_mm); full loader branch table (z / no-z / NaN / numeric-string / unparseable / out-of-range / missing columns / atomicity on failure); parfocal math for both switcher directions and the no-switcher no-op; toggle state transitions; restore-failure path; the plan-wipe regression.z (mm); load → button flips to Clear Coords, FOVs on the map; mode round-trip restores; Clear resets; acquisition from a loaded 8-well/168-FOV plan captured every image at the saved Z (verified in the run'scoordinates.csvand stage logs) and re-running reuses the plan.Design spec and implementation plan are archived externally (AI-docs/Squid/done).
🤖 Generated with Claude Code