-
Notifications
You must be signed in to change notification settings - Fork 347
Fix 7717 #7725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jenshnielsen
wants to merge
10
commits into
microsoft:main
Choose a base branch
from
jenshnielsen:fix_7717
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+297
−3
Open
Fix 7717 #7725
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
d77d31b
Add failing test based on the bug report in 7717
jenshnielsen 9e49d89
Fix 7717
jenshnielsen af9217e
Remove outdated comments
jenshnielsen eae74f9
Improve test for 7717
jenshnielsen 0cf7acd
Add infeered data to exported dataset
jenshnielsen d248e57
Only add data if shape matches
jenshnielsen 3bedb01
Add additional test for 7717
jenshnielsen 50e6dab
Add warning on invalid shape
jenshnielsen 35a5f5f
Add changelog for 7725
jenshnielsen 0b677f6
Handle multiple inferred-from parents in _add_inferred_data_vars
jenshnielsen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| Parameters using ``has_control_of`` are now correctly handled when exporting to | ||
| xarray. Controlled parameters are no longer treated as independent top-level | ||
| parameters, preventing duplicate data rows. Additionally, inferred parameters | ||
| are now included as data variables in the xarray dataset when exporting via the | ||
| pandas-based path, and a warning is logged when the inferred parameter data size | ||
| does not match its parent parameter. |
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
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
193 changes: 193 additions & 0 deletions
193
tests/dataset/test_parameter_with_setpoints_has_control.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| import logging | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| import numpy as np | ||
| import numpy.testing as npt | ||
| import xarray as xr | ||
|
|
||
| from qcodes.dataset import Measurement | ||
| from qcodes.dataset.exporters.export_to_xarray import _add_inferred_data_vars | ||
| from qcodes.parameters import ManualParameter, ParameterWithSetpoints | ||
| from qcodes.validators import Arrays | ||
|
|
||
jenshnielsen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if TYPE_CHECKING: | ||
| import pytest | ||
|
|
||
| from qcodes.dataset.experiment_container import Experiment | ||
|
|
||
|
|
||
| def test_parameter_with_setpoints_has_control(experiment: "Experiment"): | ||
astafan8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| class MySp(ParameterWithSetpoints): | ||
| def unpack_self(self, value): | ||
| res = super().unpack_self(value) | ||
| res.append((p1, p1())) | ||
| return res | ||
|
|
||
| mp_data = np.arange(10) | ||
| p1_data = np.linspace(-1, 1, 10) | ||
|
|
||
| mp = ManualParameter("mp", vals=Arrays(shape=(10,)), initial_value=mp_data) | ||
| p1 = ParameterWithSetpoints( | ||
| "p1", vals=Arrays(shape=(10,)), setpoints=(mp,), set_cmd=None | ||
| ) | ||
| p2 = MySp("p2", vals=Arrays(shape=(10,)), setpoints=(mp,), set_cmd=None) | ||
| p2.has_control_of.add(p1) | ||
|
|
||
| p1(p1_data) | ||
| p2_data = np.random.randn(10) | ||
| p2(p2_data) | ||
|
|
||
| meas = Measurement() | ||
| meas.register_parameter(p2) | ||
|
|
||
| # Only p2 should be top-level; p1 is inferred from p2 | ||
| interdeps = meas._interdeps | ||
| top_level_names = [p.name for p in interdeps.top_level_parameters] | ||
| assert top_level_names == ["p2"] | ||
|
|
||
| with meas.run() as ds: | ||
| ds.add_result((p2, p2())) | ||
|
|
||
| # Verify raw parameter data has exactly one row per parameter | ||
| raw_data = ds.dataset.get_parameter_data() | ||
| assert list(raw_data.keys()) == ["p2"], "Only p2 should be a top-level result" | ||
| for name, arr in raw_data["p2"].items(): | ||
| assert arr.shape == (1, 10), ( | ||
| f"Expected shape (1, 10) for {name}, got {arr.shape}" | ||
| ) | ||
|
|
||
| xds = ds.dataset.to_xarray_dataset() | ||
|
|
||
| # mp should be the only dimension (not a generic 'index') | ||
| assert list(xds.sizes.keys()) == ["mp"] | ||
| assert xds.sizes["mp"] == 10 | ||
|
|
||
| # mp values used as coordinate axis | ||
| npt.assert_array_equal(xds.coords["mp"].values, mp_data) | ||
|
|
||
| # p2 is the primary data variable with correct values | ||
| assert "p2" in xds.data_vars | ||
| npt.assert_array_almost_equal(xds["p2"].values, p2_data) | ||
jenshnielsen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| # p1 is included as a data variable (inferred from p2) with correct values | ||
| assert "p1" in xds.data_vars | ||
| npt.assert_array_almost_equal(xds["p1"].values, p1_data) | ||
|
|
||
| # p1 data is also retrievable from the raw parameter data | ||
| npt.assert_array_almost_equal(raw_data["p2"]["p1"].ravel(), p1_data) | ||
|
|
||
|
|
||
| def test_parameter_with_setpoints_has_control_2d(experiment: "Experiment"): | ||
| """Test that an inferred parameter with the same size as its parent | ||
| but different from the full dimension product is correctly included.""" | ||
|
|
||
| class MySp(ParameterWithSetpoints): | ||
| def unpack_self(self, value): | ||
| res = super().unpack_self(value) | ||
| res.append((p1, p1())) | ||
| return res | ||
|
|
||
| n_x = 3 | ||
| n_y = 4 | ||
| mp_x_data = np.arange(n_x, dtype=float) | ||
| mp_y_data = np.arange(n_y, dtype=float) | ||
|
|
||
| mp_x = ManualParameter("mp_x", initial_value=0.0) | ||
| mp_y = ManualParameter("mp_y", vals=Arrays(shape=(n_y,)), initial_value=mp_y_data) | ||
|
|
||
| p1 = ParameterWithSetpoints( | ||
| "p1", vals=Arrays(shape=(n_y,)), setpoints=(mp_y,), set_cmd=None | ||
| ) | ||
| p2 = MySp("p2", vals=Arrays(shape=(n_y,)), setpoints=(mp_y,), set_cmd=None) | ||
| p2.has_control_of.add(p1) | ||
|
|
||
| meas = Measurement() | ||
| meas.register_parameter(p2, setpoints=(mp_x,)) | ||
|
|
||
| p1_all = [] | ||
| p2_all = [] | ||
|
|
||
| with meas.run() as ds: | ||
| for x_val in mp_x_data: | ||
| mp_x(x_val) | ||
| p1_row = np.linspace(-1, 1, n_y) + x_val | ||
| p1(p1_row) | ||
| p2_row = np.random.randn(n_y) | ||
| p2(p2_row) | ||
| p1_all.append(p1_row) | ||
| p2_all.append(p2_row) | ||
| ds.add_result((mp_x, mp_x()), (p2, p2())) | ||
|
|
||
| p1_all_arr = np.array(p1_all) | ||
| p2_all_arr = np.array(p2_all) | ||
|
|
||
| xds = ds.dataset.to_xarray_dataset() | ||
|
|
||
| # Should have 2 dimensions: mp_x and mp_y | ||
| assert set(xds.sizes.keys()) == {"mp_x", "mp_y"} | ||
| assert xds.sizes["mp_x"] == n_x | ||
| assert xds.sizes["mp_y"] == n_y | ||
|
|
||
| # p2 is the primary data variable | ||
| assert "p2" in xds.data_vars | ||
| npt.assert_array_almost_equal(xds["p2"].values, p2_all_arr) | ||
|
|
||
| # p1 is included as a data variable (inferred from p2) | ||
| # Its size (n_x * n_y = 12) matches its parent p2's size, | ||
| # which differs from either individual dimension. | ||
| assert "p1" in xds.data_vars | ||
| npt.assert_array_almost_equal(xds["p1"].values, p1_all_arr) | ||
|
|
||
|
|
||
| def test_parameter_with_setpoints_has_control_size_mismatch_warns( | ||
| experiment: "Experiment", caplog: "pytest.LogCaptureFixture" | ||
| ) -> None: | ||
| """Test that a warning is emitted when the inferred parameter has a | ||
| different data size than its parent parameter.""" | ||
|
|
||
| class MySp(ParameterWithSetpoints): | ||
| def unpack_self(self, value): | ||
| res = super().unpack_self(value) | ||
| res.append((p1, p1())) | ||
| return res | ||
|
|
||
| mp_data = np.arange(10) | ||
|
|
||
| mp = ManualParameter("mp", vals=Arrays(shape=(10,)), initial_value=mp_data) | ||
| p1 = ParameterWithSetpoints( | ||
| "p1", vals=Arrays(shape=(10,)), setpoints=(mp,), set_cmd=None | ||
| ) | ||
| p2 = MySp("p2", vals=Arrays(shape=(10,)), setpoints=(mp,), set_cmd=None) | ||
| p2.has_control_of.add(p1) | ||
|
|
||
| p1(np.linspace(-1, 1, 10)) | ||
| p2(np.random.randn(10)) | ||
|
|
||
| meas = Measurement() | ||
| meas.register_parameter(p2) | ||
| with meas.run() as ds: | ||
| ds.add_result((p2, p2())) | ||
|
|
||
| # Build an xarray dataset and sub_dict with mismatched p1 data to | ||
| # exercise the warning path in _add_inferred_data_vars directly. | ||
|
|
||
| raw_data = ds.dataset.get_parameter_data() | ||
| sub_dict = dict(raw_data["p2"]) | ||
| # Replace p1 with wrong-sized data (5 instead of 10) | ||
| sub_dict["p1"] = np.zeros(5) | ||
|
|
||
| xr_dataset = xr.Dataset( | ||
| {"p2": (("mp",), sub_dict["p2"].ravel())}, | ||
| coords={"mp": sub_dict["mp"].ravel()}, | ||
| ) | ||
|
|
||
| with caplog.at_level( | ||
| logging.WARNING, logger="qcodes.dataset.exporters.export_to_xarray" | ||
| ): | ||
| result = _add_inferred_data_vars(ds.dataset, "p2", sub_dict, xr_dataset) | ||
|
|
||
| assert "p1" not in result.data_vars | ||
| assert any( | ||
| "Cannot add inferred parameter 'p1'" in msg and "'p2'" in msg | ||
| for msg in caplog.messages | ||
| ) | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.