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

Save InferenceData attrs #2131

Merged
merged 16 commits into from
Oct 22, 2022
Merged

Save InferenceData attrs #2131

merged 16 commits into from
Oct 22, 2022

Conversation

ahartikainen
Copy link
Contributor

@ahartikainen ahartikainen commented Oct 6, 2022

closes #2109

Description

Save attrs to root level

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@ahartikainen
Copy link
Contributor Author

@OriolAbril @canyon289 you know netCDF4 better than I do, what should be the correct way to save the attrs to the main file (not in group)?

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #2131 (114ad9e) into main (0ba14fa) will increase coverage by 0.00%.
The diff coverage is 77.77%.

@@           Coverage Diff           @@
##             main    #2131   +/-   ##
=======================================
  Coverage   90.72%   90.73%           
=======================================
  Files         118      118           
  Lines       12569    12579   +10     
=======================================
+ Hits        11403    11413   +10     
  Misses       1166     1166           
Impacted Files Coverage Δ
arviz/data/inference_data.py 82.91% <77.77%> (+0.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@canyon289
Copy link
Member

Is this way working? If so I assume its the correct way :D. If its working perhaps all we need is a test and were good to go

@sethaxen
Copy link
Member

Does the attributes variable name need to be netCDF4-specific? Can't it also be saved with e.g. Zarr?

@ahartikainen
Copy link
Contributor Author

@sethaxen very good question. It should.

Also, I think the main netcdf4 format includes a possibility for attr field at the root, but did not find a good/correct way to add it. There might be a way to do this in the docs, but atleast I did not find it.

I wonder if we actually want to create the dataset at the init and then just wrap attrs to point that?

And have one variable that will contain all the groups which will then be write/load normally?

And we just add special tag into our schema about it?

Or is this someting that Datatree will handle when it is released.

@ahartikainen
Copy link
Contributor Author

Ok, still need to figure out the zarr.

@OriolAbril
Copy link
Member

LGTM, not sure why netcdf tests are failing though would have to test locally 🤔

@ahartikainen
Copy link
Contributor Author

They fail because az.load_arviz_data("centered").to_netcdf(..) fails.

Is this the object error we see even when it should not touch the compression?

@OriolAbril
Copy link
Member

OriolAbril commented Oct 22, 2022

They fail because az.load_arviz_data("centered").to_netcdf(..) fails.

I can confirm this fails locally for me still with 0.13.0rc1 on a new env, but works with main. I think tests will pass now, but there might still be some issues when using netcdf. Not sure there is anything we can do on our side though.

@ahartikainen
Copy link
Contributor Author

Yeah, text handling is always a bit hard.

I'm not sure if xarray has changed the defaults already to S / U dtypes, but then working with these values is more or less not great.

Maybe we could add some autotransform for object type strings if we can be certain it works correctly, but tbh it is a slippery slope...

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.

Bug: to_netcdf does not save attrs
4 participants