From 6a37b36ede882d2484c97c953f5ecc63d99a47f0 Mon Sep 17 00:00:00 2001 From: wyzula-jan Date: Sat, 3 Oct 2026 16:22:23 +0200 Subject: [PATCH] fix(curve_settings): keep custom curves and DAP settings on apply --- .../settings/curve_settings/curve_tree.py | 31 ++++- .../widgets/plots/waveform/waveform.py | 76 ++++++++++--- tests/unit_tests/test_curve_settings.py | 88 +++++++++++++++ tests/unit_tests/test_waveform.py | 106 ++++++++++++++++++ 4 files changed, 283 insertions(+), 18 deletions(-) diff --git a/bec_widgets/widgets/plots/waveform/settings/curve_settings/curve_tree.py b/bec_widgets/widgets/plots/waveform/settings/curve_settings/curve_tree.py index bb16d66b..a2ee23ff 100644 --- a/bec_widgets/widgets/plots/waveform/settings/curve_settings/curve_tree.py +++ b/bec_widgets/widgets/plots/waveform/settings/curve_settings/curve_tree.py @@ -486,7 +486,20 @@ class CurveRow(QTreeWidgetItem): new_dap = ( [selected_model, *composite_models] if composite_models else selected_model ) - self.config.signal = DeviceSignal(device=device, signal=signal, dap=new_dap) + # Keep the DAP settings the tree does not edit. Fit parameter overrides belong to + # the selected model, so they only carry over while the model is unchanged. + old_signal = self.config.signal + dap_oversample = old_signal.dap_oversample if old_signal is not None else 1 + dap_parameters = None + if old_signal is not None and old_signal.dap == new_dap: + dap_parameters = old_signal.dap_parameters + self.config.signal = DeviceSignal( + device=device, + signal=signal, + dap=new_dap, + dap_oversample=dap_oversample, + dap_parameters=dap_parameters, + ) self.config.source = "dap" self.config.parent_label = parent_conf.label label_suffix = "+".join(new_dap) if isinstance(new_dap, list) else new_dap @@ -699,10 +712,22 @@ class CurveTree(BECWidget, QWidget): return new_row def send_curve_json(self): - """Send the current tree's config as JSON to the waveform, updating wavefrom.color_palette as well.""" - if self.waveform is not None: + """Send the tree's config as JSON to the waveform, applying a changed color palette too. + + Custom curves are not listed in the tree; the waveform keeps them when it loads the JSON, + and the DAP curves fitted to them are passed through unchanged. + """ + if self.waveform is not None and self.waveform.color_palette != self.color_palette: + # The palette setter recolors every curve, so only apply an actual change self.waveform.color_palette = self.color_palette data = self.export_all_curves() + if self.waveform is not None: + custom_labels = {c.name() for c in self.waveform.curves if c.config.source == "custom"} + data.extend( + c.config.model_dump() + for c in self.waveform.curves + if c.config.source == "dap" and c.config.parent_label in custom_labels + ) json_data = json.dumps(data, indent=2) if self.waveform is not None: self.waveform.curve_json = json_data diff --git a/bec_widgets/widgets/plots/waveform/waveform.py b/bec_widgets/widgets/plots/waveform/waveform.py index 65a38d68..b8b689f2 100644 --- a/bec_widgets/widgets/plots/waveform/waveform.py +++ b/bec_widgets/widgets/plots/waveform/waveform.py @@ -796,15 +796,46 @@ class Waveform(PlotBase): def curve_json(self, json_data: str): """ Load curves from a JSON string and add them to the plot, omitting custom source curves. + + The serialized curves replace all device, history and DAP curves. Custom curves hold + data that cannot be serialized, so the existing ones are kept unchanged (configuration + and data) and custom entries in the JSON are not loaded. The JSON is rejected before + any curve is removed if one of its curves is invalid, refers to a scan that is not in + the history, or would get a label that a kept custom curve or another entry already uses. """ try: curve_configs = json.loads(json_data) - self.clear_all() + custom_labels = {c.name() for c in self.curves if c.config.source == "custom"} + # Build and check every serialized curve before anything is removed, so a curve + # that cannot be added does not leave the plot half-loaded + configs = [] for cfg_dict in curve_configs: if cfg_dict.get("source") == "custom": - logger.warning(f"Custom source curve '{cfg_dict['label']}' not loaded.") + if cfg_dict.get("label") not in custom_labels: + logger.warning(f"Custom source curve '{cfg_dict.get('label')}' not loaded.") continue config = CurveConfig(**cfg_dict) + if config.source == "history": + self._resolve_history_config(config) + configs.append(config) + labels = [config.label for config in configs if config.label] + taken = custom_labels & set(labels) + if taken: + raise ValueError(f"Labels {sorted(taken)} are already used by custom curves.") + duplicates = {label for label in labels if labels.count(label) > 1} + if duplicates: + raise ValueError(f"Labels {sorted(duplicates)} are used by more than one curve.") + # Like clear_all, but keep custom curves and their colors (remove_curve recolors) + self._dap_curves = [] + self._history_curves = [] + self._sync_curves = [] + self._async_curves = [] + for curve in self.curves: + if curve.name() not in custom_labels: + self._remove_curve_by_name(curve.name()) + if self.crosshair is not None: + self.crosshair.clear_markers() + for config in configs: self._add_curve(config=config) self._refresh_alignment_state(force_readback=self._alignment_panel_visible) self._refresh_dap_signals() @@ -1124,21 +1155,9 @@ class Waveform(PlotBase): """ scan_item: ScanDataContainer | None = None if config.source == "history": - scan_item = self.get_history_scan_item( - scan_id=config.scan_id, scan_index=config.scan_number - ) - if scan_item is None: - raise ValueError( - f"Could not find scan item for history curve '{config.label}' with scan_id='{config.scan_id}' and scan_number='{config.scan_number}'." - ) - - config.scan_id = scan_item.metadata["bec"]["scan_id"] - config.scan_number = scan_item.metadata["bec"]["scan_number"] + scan_item = self._resolve_history_config(config) label = config.label - if config.source == "history": - label = f"{config.signal.device}-{config.signal.signal}-scan-{config.scan_number}" - config.label = label if not label: # Fallback label label = WidgetContainerUtils.generate_unique_name( @@ -1186,6 +1205,33 @@ class Waveform(PlotBase): return curve + def _resolve_history_config(self, config: CurveConfig) -> ScanDataContainer: + """ + Resolve the scan of a history curve config and set its scan_id, scan_number and label. + + History curves are always labelled '--scan-'. + + Args: + config (CurveConfig): Config of a history curve; updated in place. + + Returns: + ScanDataContainer: The scan item the curve reads its data from. + + Raises: + ValueError: If the scan is not found in the scan history. + """ + scan_item = self.get_history_scan_item( + scan_id=config.scan_id, scan_index=config.scan_number + ) + if scan_item is None: + raise ValueError( + f"Could not find scan item for history curve '{config.label}' with scan_id='{config.scan_id}' and scan_number='{config.scan_number}'." + ) + config.scan_id = scan_item.metadata["bec"]["scan_id"] + config.scan_number = scan_item.metadata["bec"]["scan_number"] + config.label = f"{config.signal.device}-{config.signal.signal}-scan-{config.scan_number}" + return scan_item + def _add_curve_object( self, name: str, config: CurveConfig, scan_item: ScanDataContainer | None = None ) -> Curve | None: diff --git a/tests/unit_tests/test_curve_settings.py b/tests/unit_tests/test_curve_settings.py index 0c2e1479..477eef9d 100644 --- a/tests/unit_tests/test_curve_settings.py +++ b/tests/unit_tests/test_curve_settings.py @@ -542,3 +542,91 @@ def test_curve_settings_edits_do_not_touch_live_curve_config(history_curve_setti curve_manager.export_all_curves() assert history_curve.config.model_dump() == original_config + + +################################################## +# Apply keeps custom curves and DAP settings (issues #1302, #1303) +################################################## + + +def test_apply_keeps_custom_curves(qtbot, mocked_client_with_dap): + """ + Regression for #1302: custom curves are not listed in the curve tree, but applying the + curve settings must keep them with their configuration, their data and their DAP fit. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client_with_dap) + wf.plot(arg1="bpm4i") + custom = wf.plot( + x=[1, 2, 3], + y=[4, 5, 6], + label="custom", + color="#ff0000", + pen_width=2, + pen_style="dash", + dap="GaussianModel", + ) + custom_config = custom.config.model_dump() + curve_setting = create_widget(qtbot, CurveSetting, parent=None, target_widget=wf) + + curve_setting.accept_changes() + + assert custom in wf.curves + assert custom.config.model_dump() == custom_config + x_data, y_data = custom.get_data() + assert list(x_data) == [1, 2, 3] + assert list(y_data) == [4, 5, 6] + assert sorted(c.name() for c in wf.curves) == ["bpm4i-bpm4i", "custom", "custom-GaussianModel"] + custom_dap = wf.get_curve("custom-GaussianModel") + assert custom_dap.config.parent_label == "custom" + assert custom_dap in wf._dap_curves + + +def test_apply_keeps_dap_oversample_and_parameters(qtbot, mocked_client_with_dap): + """ + Regression for #1303: applying an unrelated change in the curve settings must keep the + oversampling factor and the fit parameter overrides of a DAP curve. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client_with_dap) + wf.plot(arg1="bpm4i") + wf.add_dap_curve( + device_label="bpm4i-bpm4i", + dap_name="GaussianModel", + dap_oversample=3, + dap_parameters={"amplitude": 1.0}, + ) + dap_parameters = wf.get_curve("bpm4i-bpm4i-GaussianModel").config.signal.dap_parameters + assert dap_parameters + curve_setting = create_widget(qtbot, CurveSetting, parent=None, target_widget=wf) + + dap_row = curve_setting.curve_manager.tree.topLevelItem(0).child(0) + dap_row.color_button.set_color("#123456") # an unrelated change + curve_setting.accept_changes() + + dap_curve = wf.get_curve("bpm4i-bpm4i-GaussianModel") + assert dap_curve.config.color == "#123456" + assert dap_curve.dap_oversample == 3 + assert dap_curve.config.signal.dap_parameters == dap_parameters + + +def test_dap_model_change_drops_parameters_but_keeps_oversample(qtbot, mocked_client_with_dap): + """ + Fit parameter overrides belong to the selected model, so choosing another model drops them; + the oversampling factor does not depend on the model and is kept. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client_with_dap) + wf.plot(arg1="bpm4i") + wf.add_dap_curve( + device_label="bpm4i-bpm4i", + dap_name="GaussianModel", + dap_oversample=3, + dap_parameters={"amplitude": 1.0}, + ) + curve_tree = create_widget(qtbot, CurveTree, parent=None, waveform=wf) + dap_row = curve_tree.tree.topLevelItem(0).child(0) + + dap_row.dap_combo.fit_model_combobox.setCurrentText("LorentzModel") + exported = dap_row.export_data() + + assert exported["signal"]["dap"] == "LorentzModel" + assert exported["signal"]["dap_oversample"] == 3 + assert exported["signal"]["dap_parameters"] is None diff --git a/tests/unit_tests/test_waveform.py b/tests/unit_tests/test_waveform.py index a5e99d54..a17cf885 100644 --- a/tests/unit_tests/test_waveform.py +++ b/tests/unit_tests/test_waveform.py @@ -342,6 +342,112 @@ def test_curve_json_setter_ignores_custom(qtbot, mocked_client): assert curves[0].name() == "device_curve" +def test_curve_json_round_trip_keeps_custom_curves(qtbot, mocked_client_with_dap): + """ + Regression for #1302: custom curves cannot be serialized, so setting curve_json keeps the + existing ones (same curve, configuration and data) while it replaces the serialized curves, + including the DAP curves fitted to custom curves. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client_with_dap) + wf.plot(arg1="bpm4i") + custom = wf.plot(x=[1, 2, 3], y=[4, 5, 6], label="custom", color="#ff0000", pen_width=2) + wf.add_dap_curve(device_label="custom", dap_name="GaussianModel", dap_oversample=2) + custom_config = custom.config.model_dump() + json_str = wf.curve_json + assert [cfg["label"] for cfg in json.loads(json_str)] == ["bpm4i-bpm4i", "custom-GaussianModel"] + + wf.curve_json = json_str + + assert custom in wf.curves + assert custom.config.model_dump() == custom_config + x_data, y_data = custom.get_data() + assert list(x_data) == [1, 2, 3] + assert list(y_data) == [4, 5, 6] + assert sorted(c.name() for c in wf.curves) == ["bpm4i-bpm4i", "custom", "custom-GaussianModel"] + assert wf.get_curve("custom-GaussianModel").dap_oversample == 2 + + # A JSON without the device curve replaces it, the custom curve stays. + wf.curve_json = "[]" + assert wf.curves == [custom] + assert wf._dap_curves == [] + + +def test_curve_json_rejects_label_of_kept_custom_curve(qtbot, mocked_client): + """ + A serialized curve that would reuse the label of a kept custom curve is rejected before + any curve is removed, so the plot is not left half-loaded. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client) + device_curve = wf.plot(arg1="bpm3a") + custom = wf.plot(x=[1, 2, 3], y=[4, 5, 6], label="bpm4i-bpm4i") + clashing = { + "widget_class": "Curve", + "label": "bpm4i-bpm4i", + "source": "device", + "signal": {"device": "bpm4i", "signal": "bpm4i"}, + } + + wf.curve_json = json.dumps([clashing]) # SafeProperty logs the error + + assert wf.curves == [device_curve, custom] + + +@pytest.mark.parametrize("label", ["history_alias", None], ids=["aliased", "omitted"]) +def test_curve_json_rejects_generated_history_label_of_kept_custom_curve( + qtbot, mocked_client, scan_history_factory, label +): + """ + History curves are relabelled '--scan-' when they are added, so the + clash check must use that label, not the one in the JSON, and still fail before any curve + is removed. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client) + inject_scan_history(wf, scan_history_factory, ("hist1", 1)) + device_curve = wf.plot(arg1="bpm3a") + custom = wf.plot(x=[1, 2, 3], y=[4, 5, 6], label="bpm4i-bpm4i-scan-1") + history = { + "widget_class": "Curve", + "label": label, + "source": "history", + "scan_id": "hist1", + "signal": {"device": "bpm4i", "signal": "bpm4i"}, + } + + wf.curve_json = json.dumps([history]) # SafeProperty logs the error + + assert wf.curves == [device_curve, custom] + + +def test_curve_json_rejects_payload_before_removing_curves( + qtbot, mocked_client, scan_history_factory +): + """ + Entries that cannot be added (a history scan that is not in the history, or two entries + with the same label) are rejected before any existing curve is removed. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client) + inject_scan_history(wf, scan_history_factory, ("hist1", 1)) + device_curve = wf.plot(arg1="bpm3a") + missing_scan = { + "widget_class": "Curve", + "source": "history", + "scan_id": "not-in-history", + "signal": {"device": "bpm4i", "signal": "bpm4i"}, + } + device = { + "widget_class": "Curve", + "label": "bpm4i-bpm4i", + "source": "device", + "signal": {"device": "bpm4i", "signal": "bpm4i"}, + } + + wf.curve_json = json.dumps([device, missing_scan]) + assert wf.curves == [device_curve] + + wf.curve_json = json.dumps([device, device]) + assert wf.curves == [device_curve] + + ################################################## # Waveform widget scan logic tests ##################################################