boutproject / xBOUT

Collects BOUT++ data from parallelized simulations into xarray.
https://xbout.readthedocs.io/en/latest/
Apache License 2.0
22 stars 10 forks source link

Bugfix: Metadata doesn't exist if pre-squashed #136

Closed TomNicholas closed 4 years ago

TomNicholas commented 4 years ago

One of the recent commits caused this error when loading pre-squashed files:

    179     # Set some default settings that are only used in post-processing by xBOUT, not by
    180     # BOUT++
--> 181     ds.bout.fine_interpolation_factor = 8
    182 
    183     if info == 'terse':

/marconi_work/FUA34_SOLBOUT4/tnichola/pycode/xBOUT/xbout/boutdataset.py in fine_interpolation_factor(self, n)
    126         """
    127         ds = self.data
--> 128         ds.metadata['fine_interpolation_factor'] = n
    129         for da in ds.data_vars.values():
    130             da.metadata['fine_interpolation_factor'] = n

/marconi_work/FUA34_SOLBOUT4/tnichola/pycode/xarray/xarray/core/common.py in __getattr__(self, name)
    227                     return source[name]
    228         raise AttributeError(
--> 229             "{!r} object has no attribute {!r}".format(type(self).__name__, name)
    230         )
    231 

AttributeError: 'Dataset' object has no attribute 'metadata'

This change fixes it.

pep8speaks commented 4 years ago

Hello @TomNicholas! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2020-09-09 18:45:07 UTC
codecov-commenter commented 4 years ago

Codecov Report

Merging #136 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #136   +/-   ##
=======================================
  Coverage   77.88%   77.88%           
=======================================
  Files          14       14           
  Lines        2139     2139           
  Branches      480      480           
=======================================
  Hits         1666     1666           
  Misses        304      304           
  Partials      169      169           
Impacted Files Coverage Δ
xbout/load.py 81.23% <100.00%> (ø)

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 ec95524...98390d5. Read the comment docs.

johnomotani commented 4 years ago

I've no objections to this going in to maintain backward compatibility, but @TomNicholas if it's not much work to change, reload_boutdataset(boutdata_*.nc, pre_squashed=True) should work and also restore metadata, etc. (see the test at the bottom of this message).

I think the reload_boutdataset function is nicer for this use-case so I'd suggest actually removing the pre_squashed option from open_boutdataset in favour of always recommending reloading with reload_boutdataset.

It seemed cleaner to me at the time to have a separate function for reloading, but I think the ideal would be to auto-detect files that need 'reloading' instead of 'loading', and also auto-detect the 'pre-squashed' vs. all-variables-in-one-file cases. I might have a go at that now, and see if I can get it to work easily...

https://github.com/boutproject/xBOUT/blob/ec955242129cba57b29e863859c88ba919835655/xbout/tests/test_boutdataset.py#L799-L833

johnomotani commented 4 years ago

Replaced by #137