Skip to content
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

Don't write extra metadata for xarray in a nested dict #2934

Merged

Conversation

jenshnielsen
Copy link
Collaborator

Since this prevents export to netcdf

Fixes #2922

@codecov
Copy link

codecov bot commented Apr 16, 2021

Codecov Report

Merging #2934 (374a2c1) into master (d7d0c63) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2934      +/-   ##
==========================================
- Coverage   65.31%   65.31%   -0.01%     
==========================================
  Files         209      209              
  Lines       28038    28037       -1     
==========================================
- Hits        18313    18312       -1     
  Misses       9725     9725              

@guenp
Copy link
Contributor

guenp commented Apr 16, 2021

@jana-d FYI, the "extra_metadata" tag on exported xarray Datasets is going away in favor of supporting netCDF data export.

Copy link
Contributor

@guenp guenp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this bug! I have one minor suggestion to make the UT more complete but otherwise LGTM.

qcodes/tests/dataset/test_dataset_export.py Show resolved Hide resolved
@jenshnielsen
Copy link
Collaborator Author

@jana-d @guenp I meant to say that this is one proposed solution. We could also merge all the data in extra_metadata (from all tags into one json string) but I think this would be cleaner? @astafan8 Any opinion

@jana-d
Copy link
Contributor

jana-d commented Apr 19, 2021

I think the proposed solution is good, there is no need for the extra_metadata tag. Also agree that not merging everything into one json string might be cleaner, but happy either way.

@jenshnielsen jenshnielsen added this to the 0.25.0 milestone Apr 20, 2021
Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with this solution as well.

the point of using the extra_metadata key to group "all aadditional metadata" was to separate it from the must-have/-be attributes, but seems to give more headache than benefit

@jenshnielsen jenshnielsen merged commit ae70642 into microsoft:master Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dataset with nested metadata throws TypeError when exporting to NetCDF file
4 participants