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 and reload #124

Merged
merged 12 commits into from
May 31, 2020
Merged

save and reload #124

merged 12 commits into from
May 31, 2020

Conversation

johnomotani
Copy link
Collaborator

@johnomotani johnomotani commented May 22, 2020

Changes to allow a BoutDataset to be saved to a file and re-loaded, replicating the state of the original BoutDataset.

Activates previously-skipped tests of save() as well as adding tests using reload_boutdataset().

Needs to deal with options. Previously it was an error to save a BoutDataset that had non-empty options. Now just warn instead - temporary workaround (until #94, #97 are resolved) as now the BoutDataset can be saved, but the input file must be given again when re-loading the BoutDataset.

Previously it was an error to save a BoutDataset that had non-empty
options. Now just warn instead - temporary workaround as now the
BoutDataset can be saved, but the input file must be given again when
re-loading the BoutDataset.
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2020

Codecov Report

Merging #124 into master will increase coverage by 2.86%.
The diff coverage is 80.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   50.29%   53.16%   +2.86%     
==========================================
  Files          11       11              
  Lines        1175     1217      +42     
  Branches      234      246      +12     
==========================================
+ Hits          591      647      +56     
+ Misses        512      496      -16     
- Partials       72       74       +2     
Impacted Files Coverage Δ
xbout/geometries.py 74.25% <0.00%> (ø)
xbout/boutdataset.py 77.01% <76.92%> (+10.15%) ⬆️
xbout/load.py 77.51% <83.33%> (+2.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 741c66a...75c1e90. Read the comment docs.

@johnomotani johnomotani added the bugfix Fix for a bug label May 24, 2020
To restore the metadata when reloading a saved Dataset, will need to
know which attributes are 'metadata', so when converting a dict to
attrs, include a prefix with the name of the dict.
New function reload_boutdataset() loads a BoutDataset from a netCDF file
created by `ds.bout.save()`. Necessary because for example dimensions
may be renamed when adding geometry, and metadata must be converted from
dict to attributes to be saved, so reloading the saved Dataset is not
the same as initial loading from BOUT++ output files.
Saving as float32 and loading still does not work.
@johnomotani johnomotani changed the title Temporary workaround: warn instead of throw if options present when saving save and reload May 25, 2020
@johnomotani johnomotani added the enhancement New feature or request label May 25, 2020
@johnomotani
Copy link
Collaborator Author

As noted in the updated PR description at the top, this PR now updates the saving functionality, and adds reload_boutdataset. Also turns on some tests for saving that were previously skipped, and adds some new tests of reloading.

Copy link
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

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

LGTM, just small change to using mutable object as a default value

xbout/load.py Outdated Show resolved Hide resolved
@johnomotani
Copy link
Collaborator Author

Thanks for the review @ZedThree. Mutable default fixed now, and in a few extra places too!

Copy link
Collaborator

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

This looks good, only very minor comments

dict_to_attrs(to_save, 'metadata')
# Must do this for all variables and coordinates in dataset too
for varname, da in chain(to_save.data_vars.items(), to_save.coords.items()):
try:
dict_to_attrs(da, key='metadata')
dict_to_attrs(da, 'metadata')
except KeyError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What would happen if there isn't any metadata?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If there's no metadata on a variable then there will be a KeyError and that nothing is done for that variable.

xbout/geometries.py Outdated Show resolved Hide resolved
@johnomotani
Copy link
Collaborator Author

Tests are now failing for the cases that use the latest versions of installed packages. I tried upgrading to the latest released xarray and dask on my system, but cannot reproduce the error. I'm not sure what to try except messing restricting the package versions used by Travis to try and isolate the problem. If anyone has a smarter idea it would be very welcome! Otherwise this will have to wait until I (or any volunteer!) have some time to track down the test failures :-(

@johnomotani johnomotani dismissed ZedThree’s stale review May 31, 2020 13:13

Was fixed, just not by merging the suggested change (some other lines needed changing too).

@johnomotani johnomotani merged commit 21ca594 into master May 31, 2020
@johnomotani johnomotani deleted the skip-options-write branch May 31, 2020 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix for a bug enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants