search

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

Define Regions and add method to get BoutDataArrays in a region #107

Closed johnomotani closed 4 years ago

johnomotani commented 4 years ago

It will be useful to operate on or plot variables in a region, within which the grid is contiguous. This PR provides a Region class to define regions (indices of their boundaries, connections to neighbouring regions). It also adds a method BoutDataArray.fromRegion() to get a variable in a region, including guard cells correctly filled from the connections of the region (not the adjacent points in the global array).

Includes #106, because it uses the join argument to xr.concat.

This is a fairly big PR, but only 835 of the lines added are 'code', the rest are for the tests.

pep8speaks commented 4 years ago

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

Line 999:90: E501 line too long (90 > 89 characters) Line 1014:52: E128 continuation line under-indented for visual indent Line 1030:90: E501 line too long (90 > 89 characters) Line 1099:90: E501 line too long (90 > 89 characters) Line 1114:52: E128 continuation line under-indented for visual indent Line 1128:90: E501 line too long (90 > 89 characters) Line 1142:90: E501 line too long (90 > 89 characters) Line 1158:52: E128 continuation line under-indented for visual indent Line 1174:90: E501 line too long (90 > 89 characters) Line 1244:90: E501 line too long (90 > 89 characters) Line 1259:52: E128 continuation line under-indented for visual indent Line 1273:90: E501 line too long (90 > 89 characters)

Comment last updated at 2020-06-18 15:43:30 UTC
johnomotani commented 4 years ago

Tests with minimum versions are failing because I am relying on xr.concat() to take the attributes from the first variable. Unfortunately this was introduced in xarray-0.15.0, and I don't think we want to require that yet... I'll have a think about a work-around.

johnomotani commented 4 years ago

Something's also gone wrong with the 'standard package versions' form of the tests, that's causing errors in the imports, possibly something netCDF4 related. I'm hoping that that's just the Travis environment being messed up and it might magically fix itself. Since the checks passed for #106, it shouldn't be due to the versions of xarray/netCDF4 we're using, so I can't see how the problem is caused by us.

johnomotani commented 4 years ago

The tests for fromRegion are pretty slow. I think this is probably because they all use bout_xyt_example_files to create a set of BOUT.dmp.*.nc files and then load them. It would be nice for testing to be able to open a (nested-)list of Datasets instead of filenames. It looks to me though like a simple impelmentation of that would require xarray changes, because it'd be nice to keep the logic of _trim and the combining of different parts into the final Dataset the same. That would need xr.open_mfdataset() to handle being passed a list of Datasets, but then I think everything in xBOUT should just work, except for some changes in the tests to get/pass a list of Datasets instead of filepath glob.

@TomNicholas do you think that would be a useful feature for xarray to have?

johnomotani commented 4 years ago

Unfortunately there are a lot of tests, because different topologies are basically a list of special cases, and I think it's necessary to test them all. I found in writing the tests that the limiter configuration is a particular headache, because it has neighbouring regions where one has y-boundaries and the other does not, and fromRegion() treats boundary cells and guard cells differently.

johnomotani commented 4 years ago

Something's also gone wrong with the 'standard package versions' form of the tests, that's causing errors in the imports, possibly something netCDF4 related. I'm hoping that that's just the Travis environment being messed up and it might magically fix itself. Since the checks passed for #106, it shouldn't be due to the versions of xarray/netCDF4 we're using, so I can't see how the problem is caused by us.

I just made a new branch is-travis-broken which is ahead of master by one commit that adds a couple of lines of whitespace to .gitignore. All tests passed on master, but the python3.7 tests failed on is-travis-broken, so I think those python3.7 fails are a Travis problem which will hopefully fix itself eventually.

codecov-io commented 4 years ago

Codecov Report

Merging #107 into master will increase coverage by 18.02%. The diff coverage is 80.94%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #107       +/-   ##
===========================================
+ Coverage   50.29%   68.32%   +18.02%     
===========================================
  Files          11       12        +1     
  Lines        1175     1427      +252     
  Branches      234      280       +46     
===========================================
+ Hits          591      975      +384     
+ Misses        512      366      -146     
- Partials       72       86       +14
Impacted Files Coverage Δ
xbout/load.py 79.84% <ø> (+4.65%) :arrow_up:
xbout/plotting/plotfuncs.py 6.54% <0%> (-0.61%) :arrow_down:
xbout/plotting/utils.py 16.66% <0%> (+9.75%) :arrow_up:
xbout/boutdataarray.py 80.32% <100%> (+5.59%) :arrow_up:
xbout/boutdataset.py 67.97% <100%> (+1.11%) :arrow_up:
xbout/geometries.py 75.47% <100%> (+1.21%) :arrow_up:
xbout/plotting/animate.py 42.02% <28.57%> (ø) :arrow_up:
xbout/region.py 91.05% <91.05%> (ø)

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 3062f96...71c0160. Read the comment docs.

johnomotani commented 4 years ago

Increasing the version of numpy for the Travis tests to >=1.16.0 (I think it was using 1.15.4 before) seems to have fixed the crashes with python3.7. Still don't understand why it was failing in the first place, but this seems OK as a workaround.

johnomotani commented 4 years ago

@TomNicholas I think this PR is ready for review/merge now.

codecov-commenter commented 4 years ago

Codecov Report

Merging #107 into master will increase coverage by 17.06%. The diff coverage is 80.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #107       +/-   ##
===========================================
+ Coverage   53.16%   70.22%   +17.06%     
===========================================
  Files          11       12        +1     
  Lines        1217     1488      +271     
  Branches      246      299       +53     
===========================================
+ Hits          647     1045      +398     
+ Misses        496      353      -143     
- Partials       74       90       +16
Impacted Files Coverage Δ
xbout/plotting/plotfuncs.py 6.54% <0.00%> (-0.61%) :arrow_down:
xbout/plotting/utils.py 16.66% <0.00%> (+9.75%) :arrow_up:
xbout/plotting/animate.py 42.02% <28.57%> (ø)
xbout/geometries.py 74.10% <76.92%> (-0.16%) :arrow_down:
xbout/region.py 90.81% <90.81%> (ø)
xbout/boutdataarray.py 80.00% <100.00%> (+5.26%) :arrow_up:
xbout/boutdataset.py 77.90% <100.00%> (+0.88%) :arrow_up:
xbout/load.py 81.66% <100.00%> (+4.14%) :arrow_up:

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 21ca594...ab4f52a. Read the comment docs.