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.58% +0.03%
==========================================
Files 333 333
Lines 32232 32284 +52
==========================================
+ Hits 19516 19559 +43
- Misses 12716 12725 +9 ☔ 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"?
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.
There was a problem hiding this comment.
Pull request overview
Fixes QCoDeS dataset export behavior related to issue #7717, where ParameterWithSetpoints parameters involved in has_control_of/inference relationships could cause to_xarray_dataset() to export ravelled (ungridded) data instead of a properly gridded dataset.
Changes:
- Add regression tests covering 1D and 2D
ParameterWithSetpoints+has_control_ofinference export behavior, plus a warning-path test. - Adjust
InterDependencies_.top_level_parametersto avoid treating inferred parameters as independent top-level parameters. - Extend xarray export to include inferred parameters as data variables when exporting via the pandas-index path, warning when inferred data sizes don’t match the parent parameter.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/dataset/test_parameter_with_setpoints_has_control.py |
Adds regression tests reproducing #7717 and validating inferred parameters appear correctly in xarray export. |
src/qcodes/dataset/exporters/export_to_xarray.py |
Adds _add_inferred_data_vars and integrates it into the pandas-index export paths to include inferred parameters. |
src/qcodes/dataset/descriptions/dependencies.py |
Updates top_level_parameters to exclude parameters inferred from others from dependency top-level candidates. |
| inferred_from_params = interdeps.inferences.get(inf, ()) | ||
| if len(inferred_from_params) == 0: | ||
| continue | ||
| parent_name = inferred_from_params[0].name | ||
| if parent_name not in sub_dict: | ||
| continue | ||
| expected_size = sub_dict[parent_name].ravel().shape[0] | ||
| if flat.shape[0] == expected_size: |
There was a problem hiding this comment.
_add_inferred_data_vars assumes an inferred parameter has exactly one inferred_from parent by taking inferred_from_params[0]. ParamSpecs can be inferred from multiple parameters (e.g. inferred_from=[p1, p2]), and picking the first element can be nondeterministic and can cause inferred variables to be skipped or warned incorrectly. Consider iterating over all inferred-from parents (that are present in sub_dict) and using the first one whose flattened size matches, or otherwise logging an explicit warning/choosing a deterministic parent (e.g. prefer the top-level measured parameter if present).
There was a problem hiding this comment.
The logic was updated to check all inferred from parameters and export if one of them matches. Perhaps it would be more correct if all of them match? @astafan8 what do you think?
There was a problem hiding this comment.
Perhaps it would be more correct if all of them match?
that depends on whether we expect the data shapes for the inferred from parameters to be the same as of the parent. do we? that's not obvious to me.
Instead of assuming a single parent by taking inferred_from_params[0], iterate over all inferred-from parents and use the first one whose flattened data size matches. Log a warning listing all available parents when no match is found. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.