feat: Wellplate calibration improvements - center point method and parameter editing #490
Conversation
…ellplate calibration - Add new calibration method that uses single center point instead of 3 edge points - Recommended for small wells (384, 1536 well plates) - Auto-selected when calibrating 384 or 1536 well plates - Includes manual well size input since it can't be calculated from one point - Add ability to update existing format parameters without recalibration - Shows current rows, columns, well spacing, and well size - All fields are editable with an "Update Parameters" button - Values auto-load when selecting different existing formats Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When a user names their custom format with "well plate" in the name (e.g., "xx well plate"), avoid appending " well plate" again in display text and messages. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds two major features to the wellplate calibration system: a new center point calibration method optimized for small wells (384 and 1536 well plates), and the ability to update format parameters without performing recalibration. It also fixes a display bug where custom format names containing "well plate" would show duplicate suffixes.
Changes:
- Added center point calibration method as an alternative to the 3-edge-points method, with auto-selection for 384/1536 well plates
- Implemented parameter editing UI that allows updating rows, columns, spacing, and well size for existing formats without recalibration
- Fixed duplicate "well plate" suffix issue in format display names across multiple locations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
software/control/widgets.py
Outdated
| self.existing_well_size_input.setValue(settings.get("well_size_mm", 6.0)) | ||
|
|
||
| # Also update center point well size input | ||
| self.center_well_size_input.setValue(settings.get("well_size_mm", 3.0)) |
There was a problem hiding this comment.
Inconsistent default value used for center_well_size_input. The value should be taken from settings.get("well_size_mm") without a hardcoded fallback of 3.0. The line above (13349) correctly retrieves the existing well size for the existing_well_size_input, but this line uses a hardcoded 3.0 instead of using the same value. This could lead to confusion where the two inputs show different values for the same format.
| self.existing_well_size_input.setValue(settings.get("well_size_mm", 6.0)) | |
| # Also update center point well size input | |
| self.center_well_size_input.setValue(settings.get("well_size_mm", 3.0)) | |
| well_size = settings.get("well_size_mm", 6.0) | |
| self.existing_well_size_input.setValue(well_size) | |
| # Also update center point well size input | |
| self.center_well_size_input.setValue(well_size) |
| self.calibration_method_group = QGroupBox("Calibration Method") | ||
| calibration_method_layout = QVBoxLayout() | ||
|
|
||
| self.method_button_group = QButtonGroup(self) | ||
| self.edge_points_radio = QRadioButton("3 Edge Points (recommended for large wells)") | ||
| self.center_point_radio = QRadioButton("Center Point (recommended for small wells)") | ||
| self.method_button_group.addButton(self.edge_points_radio) | ||
| self.method_button_group.addButton(self.center_point_radio) | ||
| self.edge_points_radio.setChecked(True) | ||
|
|
||
| calibration_method_layout.addWidget(self.edge_points_radio) | ||
| calibration_method_layout.addWidget(self.center_point_radio) | ||
| self.calibration_method_group.setLayout(calibration_method_layout) | ||
| left_layout.addWidget(self.calibration_method_group) |
There was a problem hiding this comment.
The calibration method selection (calibration_method_group) and calibration points UI (points_widget and center_point_widget) are always visible, even when the "Update Parameters" button is being used. When a user selects "Calibrate Existing Format" mode, they have two workflows: (1) update parameters without calibration, or (2) recalibrate. The calibration method and points UI should only be visible when the user intends to perform calibration, not when updating parameters. Consider hiding these widgets in toggle_input_mode() when calibrate_format_radio is checked, and only showing them when the user indicates they want to calibrate (perhaps with a separate toggle or by keeping them visible but making it clear they're optional).
software/control/widgets.py
Outdated
| # Avoid duplicating "well plate" if already in the name | ||
| display_name = str(format_) | ||
| if "well plate" not in display_name.lower(): | ||
| display_name = f"{format_} well plate" | ||
| self.existing_format_combo.addItem(display_name, format_) |
There was a problem hiding this comment.
The logic for avoiding duplicate "well plate" suffix is repeated in multiple places (lines 13315-13318, 13431-13433, 13462-13464, 13555-13557, 13575-13577). Consider extracting this into a helper method to reduce duplication and ensure consistent behavior. For example, a method like format_display_name(format_) that returns the properly formatted string.
| def toggle_calibration_method(self): | ||
| """Toggle between 3 edge points and center point calibration methods.""" | ||
| if self.edge_points_radio.isChecked(): | ||
| self.points_widget.show() | ||
| self.center_point_widget.hide() | ||
| else: | ||
| self.points_widget.hide() | ||
| self.center_point_widget.show() | ||
| self.update_calibrate_button_state() |
There was a problem hiding this comment.
When toggling between calibration methods, the calibration points are not reset. If a user sets edge points, then switches to center point method, then switches back to edge points, the previously set edge points are still there. This could lead to confusion. Consider calling reset_calibration_points() in toggle_calibration_method() to ensure a clean state when switching methods, or clearly indicate to the user that previous points are preserved.
software/control/widgets.py
Outdated
| # Auto-select center point method for 384 and 1536 well plates | ||
| if selected_format in (384, 1536): | ||
| self.center_point_radio.setChecked(True) | ||
| else: | ||
| self.edge_points_radio.setChecked(True) |
There was a problem hiding this comment.
The auto-selection logic for center point method checks if selected_format is exactly 384 or 1536 (integer values). However, if custom formats are created with names like "384_custom" or other non-integer formats, they won't match this check. Consider whether this is the intended behavior. If custom 384/1536 well formats should also use center point method, the logic needs to be more flexible (e.g., parsing the number from the format name or checking a property of the format).
software/control/widgets.py
Outdated
| # Update the WellplateFormatWidget's combo box to reflect the newly calibrated format | ||
| self.wellplateFormatWidget.populate_combo_box() |
There was a problem hiding this comment.
Code references variables from both code paths without proper scoping. At this point in the code, if the existing format path was taken, 'name' is not defined and will cause a NameError on line 13583 (though unlikely to occur in practice since the radio button state is checked). Consider restructuring to explicitly handle both cases or initializing variables at the start of the method.
|
|
||
| self.corners[index] = (x, y) | ||
| self.cornerLabels[index].setText(f"Point {index+1}: ({x:.2f}, {y:.2f})") | ||
| self.cornerLabels[index].setText(f"Point {index+1}: ({x:.3f}, {y:.3f})") |
There was a problem hiding this comment.
Coordinate precision changed from 2 to 3 decimal places. While this change increases precision for displaying coordinates, it's inconsistent with the rest of the codebase. If this precision change is intentional for small well plates where sub-millimeter precision is important, it should be applied consistently. Otherwise, consider whether .2f is sufficient for the use case.
| self.cornerLabels[index].setText(f"Point {index+1}: ({x:.3f}, {y:.3f})") | |
| self.cornerLabels[index].setText(f"Point {index+1}: ({x:.2f}, {y:.2f})") |
software/control/widgets.py
Outdated
|
|
||
| # Get the existing format settings | ||
| existing_settings = WELLPLATE_FORMAT_SETTINGS[selected_format] | ||
|
|
||
| print(f"Updating existing format {selected_format} well plate") | ||
| # Avoid duplicating "well plate" if already in the name | ||
| log_name = str(selected_format) | ||
| if "well plate" not in log_name.lower(): | ||
| log_name = f"{selected_format} well plate" | ||
| print(f"Updating existing format {log_name}") | ||
| print( | ||
| f"OLD: 'a1_x_mm': {existing_settings['a1_x_mm']}, 'a1_y_mm': {existing_settings['a1_y_mm']}, 'well_size_mm': {existing_settings['well_size_mm']}" | ||
| ) |
There was a problem hiding this comment.
When recalibrating an existing format, the pixel coordinates (a1_x_pixel and a1_y_pixel) are not updated in the settings dictionary. While the mm coordinates are updated in the updated_settings dictionary, the pixel coordinates remain unchanged from their previous values. This creates an inconsistency between mm and pixel representations. The pixel coordinates should be recalculated using the same scale factor (1 / 0.084665) as done for new formats on lines 13507-13509.
software/control/widgets.py
Outdated
| # Get the new values | ||
| new_rows = self.existing_rows_input.value() | ||
| new_cols = self.existing_cols_input.value() | ||
| new_spacing = self.existing_spacing_input.value() | ||
| new_well_size = self.existing_well_size_input.value() | ||
|
|
||
| # Get existing settings | ||
| existing_settings = WELLPLATE_FORMAT_SETTINGS.get(selected_format, {}) | ||
|
|
||
| # Avoid duplicating "well plate" if already in the name | ||
| log_name = str(selected_format) | ||
| if "well plate" not in log_name.lower(): | ||
| log_name = f"{selected_format} well plate" | ||
| print(f"Updating parameters for {log_name}") | ||
| print( | ||
| f"OLD: rows={existing_settings.get('rows')}, cols={existing_settings.get('cols')}, " | ||
| f"spacing={existing_settings.get('well_spacing_mm')}, well_size={existing_settings.get('well_size_mm')}" | ||
| ) | ||
| print(f"NEW: rows={new_rows}, cols={new_cols}, spacing={new_spacing}, well_size={new_well_size}") | ||
|
|
||
| # Update the settings | ||
| WELLPLATE_FORMAT_SETTINGS[selected_format].update( | ||
| { | ||
| "rows": new_rows, | ||
| "cols": new_cols, | ||
| "well_spacing_mm": new_spacing, | ||
| "well_size_mm": new_well_size, | ||
| } | ||
| ) |
There was a problem hiding this comment.
The update_existing_parameters method lacks validation to ensure the updated parameters are physically reasonable. Specifically, there's no check that well_size is less than well_spacing, which would be required for wells not to overlap. Consider adding validation such as: if new_well_size >= new_spacing: show a warning that well size must be smaller than well spacing.
| self.edge_points_label.setAlignment(Qt.AlignCenter) | ||
| points_layout.addWidget(self.edge_points_label, 0, 0, 1, 2) | ||
| for i in range(1, 4): | ||
| label = QLabel(f"Point {i}: N/A") |
There was a problem hiding this comment.
Inconsistent initial label text for corner points. In initUI, the labels are initialized as "Point {i}: N/A" (line 13126, where i ranges from 1-3), but in reset_calibration_points they're set to "Point {i+1}: Not set" (line 13372, where i ranges from 0-2). While the numbering ends up the same, the text differs between "N/A" and "Not set". Consider using consistent text, such as always using "Not set" for clarity.
| label = QLabel(f"Point {i}: N/A") | |
| label = QLabel(f"Point {i}: Not set") |
- Remove duplicate signal connection for toggle_calibration_method - Use consistent fallback values for well_size_mm (both inputs use same value) - Add error handling with traceback logging to update_existing_parameters() - Add specific LinAlgError handling for collinear points in calibrate() - Add traceback logging to general exception handler in calibrate() - Add docstring to calibrate() method explaining the two modes and methods - Add rationale comment for auto-selecting center point on 384/1536 plates Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Extract _format_display_name() helper to consolidate repeated suffix logic - Extract _get_calibration_data() to unify validation for both calibration methods - Split calibrate() into focused methods: _calibrate_new_format(), _calibrate_existing_format(), _finish_calibration() - Remove unused exception variable in LinAlgError handler Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…zing
- Fix auto-selection check to use string keys ("384 well plate", "1536 well plate")
instead of integers (384, 1536) to match WELLPLATE_FORMAT_SETTINGS keys
- Set minimum dialog height after initialization to prevent height changes
when toggling between UI modes
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove premature setWellplateSettings() calls that were emitting signals before wellplate_format was updated, causing KeyError when accessing settings["rows"] - Set fixed minimum height (700px) for calibration dialog to prevent UI elements from being squeezed when toggling modes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Rows and columns are fundamental properties of a wellplate format that should not be changed via parameter updates. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Wrap new format inputs in container widget to eliminate spacing when switching to existing format mode - Hide well size input under center point when calibrating existing formats (uses Format Parameters section instead) - Simplify toggle_input_mode using setVisible() with boolean - Reduce minimum dialog height to 580px Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Add new center point calibration method for small wells (384, 1536 well plates)
Add ability to update existing format parameters without recalibration
Fix bug where custom format names containing "well plate" would display as "xx well plate well plate"
Test plan
🤖 Generated with Claude Code