Conversation
|
@astafan8 converted to draft until it actually fixes the bug |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7725 +/- ##
==========================================
+ Coverage 60.54% 60.55% +0.01%
==========================================
Files 333 333
Lines 32227 32261 +34
==========================================
+ Hits 19513 19537 +24
- Misses 12714 12724 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
astafan8
left a comment
astafan8
left a comment
There was a problem hiding this comment.
great to see this fixed! And "add a news fragment"?
|
|
||
| # 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) |
There was a problem hiding this comment.
shall an assrtion be added for p1 in the xarray dataset as well?
| xr_dataset[inf.name] = ( | ||
| dims, | ||
| flat.reshape(tuple(xr_dataset.sizes[d] for d in dims)), | ||
| ) |
There was a problem hiding this comment.
We should probably warn if this is not the case
There was a problem hiding this comment.
Pull request overview
This PR addresses #7717, where a dataset export to xarray becomes ungridded when two ParameterWithSetpoints share the same setpoints and one parameter is inferred/controlled by the other. The intent is to ensure the export stays gridded and includes the inferred parameter data alongside the measured top-level parameter.
Changes:
- Adds a regression test reproducing the
ParameterWithSetpoints.has_control_of+ shared setpoints scenario. - Updates
InterDependencies_.top_level_parametersto exclude parameters that are inferred-from others from being considered dependency top-level parameters. - Extends the xarray export (pandas-index path) to add inferred parameters as xarray data variables.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/dataset/test_parameter_with_setpoints_has_control.py |
Regression test ensuring gridded xarray export includes inferred p1 as a data var while keeping only p2 top-level. |
src/qcodes/dataset/exporters/export_to_xarray.py |
Adds _add_inferred_data_vars to include inferred parameters in xarray output when exporting via pandas index. |
src/qcodes/dataset/descriptions/dependencies.py |
Adjusts top-level parameter detection to prevent inferred parameters from being treated as independent top-level parameters. |
You can also share your feedback on Copilot code review. Take the survey.
| interdeps = dataset.description.interdeps | ||
| meas_paramspec = interdeps.graph.nodes[name]["value"] | ||
| _, deps, inferred = interdeps.all_parameters_in_tree_by_group(meas_paramspec) | ||
|
|
||
| dep_names = {dep.name for dep in deps} | ||
| dims = tuple(d for d in xr_dataset.dims) | ||
|
|
||
| for inf in inferred: | ||
| if inf.name in dep_names: | ||
| continue | ||
| if inf.name in xr_dataset: | ||
| continue | ||
| if inf.name not in sub_dict: | ||
| continue | ||
|
|
||
| inf_data = sub_dict[inf.name] | ||
| if inf_data.dtype == np.dtype("O"): | ||
| try: | ||
| flat = np.concatenate(inf_data) | ||
| except ValueError: | ||
| flat = inf_data.ravel() | ||
| else: | ||
| flat = inf_data.ravel() | ||
|
|
||
| # Only add if the data length matches the existing dataset size | ||
| expected_size = 1 | ||
| for d in dims: | ||
| expected_size *= xr_dataset.sizes[d] | ||
| if flat.shape[0] == expected_size: | ||
| xr_dataset[inf.name] = ( | ||
| dims, | ||
| flat.reshape(tuple(xr_dataset.sizes[d] for d in dims)), | ||
| ) |
Uh oh!
There was an error while loading. Please reload this page.