boutproject / xBOUT

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

Ensure data is converted to strings #238

Closed dschwoerer closed 2 years ago

dschwoerer commented 2 years ago

This is required for comparisons, as bytes != string.

johnomotani commented 2 years ago

Is this related to #229?

dschwoerer commented 2 years ago

Sorry, was not aware of #229 xarray is 2022.3.0

I don't think so. My issue was the following:

ds = xbout.open_boutdataset(...)
grid = xr.open_dataset(...)
assert ds.metadata["grid_id"] == grid["id"]

The assertion failed, when it should not have failed, imho. This fixes the issue, by converting everything to strings.

codecov-commenter commented 2 years ago

Codecov Report

Merging #238 (45b640e) into master (9f8e094) will decrease coverage by 0.03%. The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
- Coverage   75.04%   75.00%   -0.04%     
==========================================
  Files          15       15              
  Lines        2781     2785       +4     
  Branches      669      671       +2     
==========================================
+ Hits         2087     2089       +2     
- Misses        452      453       +1     
- Partials      242      243       +1     
Impacted Files Coverage Δ
xbout/load.py 77.48% <50.00%> (-0.35%) :arrow_down:
xbout/utils.py 82.54% <100.00%> (+0.04%) :arrow_up:

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

johnomotani commented 2 years ago

I've been trying to remember why we didn't want to merge #229. I think it was just because it wasn't necessary, rather than any actual objection. This change does change the type of some data, but as far as I can think, it will always be more useful, and more likely to behave as expected in Python, to have the variables be str rather than bytes. The NetCDF file written by BoutDataset.save() is already so different from the NetCDF files that BOUT++ or bout-squashoutput produce (e.g. metadata stored as attributes, etc.) that changing these types as well does not seem important.

So I think this PR should go in. @dschwoerer could you just copy the additions to tests from #229 please? Then I think this is good to go.

ZedThree commented 2 years ago

+1 I agree it's better to have str than bytes