Skip to content

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

Closed
TomNicholas wants to merge 3 commits intomasterfrom
pre_squashed_bugfix
Closed

Bugfix: Metadata doesn't exist if pre-squashed#136
TomNicholas wants to merge 3 commits intomasterfrom
pre_squashed_bugfix

Conversation

@TomNicholas
Copy link
Collaborator

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
Copy link

pep8speaks commented Sep 9, 2020

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! 🍻

Comment last updated at 2020-09-09 18:45:07 UTC

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2020

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
johnomotani previously approved these changes Sep 9, 2020
Copy link
Collaborator

@johnomotani johnomotani left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @TomNicholas!

May indicate we need a test of pre_squashed=True... I'll have a think about it.

@johnomotani johnomotani dismissed their stale review September 9, 2020 19:02

May be better to use reload_boutdataset?

@johnomotani
Copy link
Collaborator

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...

def test_reload_separate_variables(
self, tmpdir_factory, bout_xyt_example_files, geometry
):
if geometry is not None:
grid = "grid"
else:
grid = None
path = bout_xyt_example_files(
tmpdir_factory, nxpe=4, nype=1, nt=1, grid=grid, write_to_disk=True
)
if grid is not None:
gridpath = str(Path(path).parent) + "/grid.nc"
else:
gridpath = None
# Load it as a boutdataset
original = open_boutdataset(
datapath=path,
inputfilepath=None,
geometry=geometry,
gridfilepath=gridpath,
)
# Save it to a netCDF file
savepath = str(Path(path).parent) + '/temp_boutdata.nc'
original.bout.save(savepath=savepath, separate_vars=True)
# Load it again
savepath = str(Path(path).parent) + '/temp_boutdata_*.nc'
recovered = reload_boutdataset(savepath, pre_squashed=True)
# Compare
xrt.assert_identical(recovered, original)

@johnomotani
Copy link
Collaborator

Replaced by #137

@johnomotani johnomotani deleted the pre_squashed_bugfix branch September 10, 2020 23:31
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.

4 participants