Implement 1D animations

[johnomotani]

Could probably make this fancier, e.g. to allow over-plotting of multiple lines, but it works!

[pep8speaks]

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

• In the file xbout/tests/test_animate.py:

Line 22:58: E261 at least two spaces before inline comment

• In the file xbout/tests/test_load.py:

Line 171:74: E231 missing whitespace after ','
Line 171:76: E231 missing whitespace after ','
Line 171:78: E231 missing whitespace after ','
Line 200:43: E231 missing whitespace after ','
Line 200:45: E231 missing whitespace after ','
Line 200:47: E231 missing whitespace after ','
Line 231:54: E231 missing whitespace after ','
Line 231:56: E231 missing whitespace after ','
Line 231:58: E231 missing whitespace after ','
Line 250:28: E251 unexpected spaces around keyword / parameter equals
Line 250:30: E251 unexpected spaces around keyword / parameter equals
Line 341:90: E501 line too long (90 > 89 characters)

Comment last updated at 2019-10-23 16:04:00 UTC

[TomNicholas]

Thanks!

I will have a look at this properly later, but you will be interested in this PR to xarray, which I hope one day can can supplant this.

[codecov-io]

Codecov Report

Merging #36 into master will decrease coverage by 4.3%.
The diff coverage is 3.44%.

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   48.03%   43.72%   -4.31%     
==========================================
  Files           6        6              
  Lines         254      279      +25     
  Branches       51       59       +8     
==========================================
  Hits          122      122              
- Misses        122      147      +25     
  Partials       10       10

Impacted Files	Coverage Δ
xbout/boutdataarray.py	26.66% <ø> (ø)	⬆️
xbout/plotting/animate.py	9.23% <3.44%> (-5.77%)	⬇️

---

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 cc503e7...317420e. Read the comment docs.

[johnomotani]

Note the test failures seem to be due to an xarray bug pydata/xarray#3401

[johnomotani]

Thanks to @rdoyle45, the xarray issue causing the test failures is now fixed in xarray/master, so the tests should pass once the next xarray release comes out (whenever that is). In the meantime, the tests pass on my computer with the older xarray/1.3.0, so I think this PR is ready for review/merge.

I guess we could change the test environment to use a specific version of xarray that works, but I think it's good to keep using the latest version to pick up issues like this, so I'm in favour of leaving the test setup as-is, and just verifying the checks manually.

[johnomotani]

I need to update the test data files in xbout/tests/data/dump_files to add jyseps1_2, etc. to get the tests to pass. The files are ~21MB - I think it might be better to remove the .nc files and replace them with a simple python script/function to generate the required files. Thoughts?

[TomNicholas]

Yes, if we can automate the generation of test data we should. Do they need to be 21MB?

…

On Mon, 21 Oct 2019, 14:55 johnomotani, ***@***.***> wrote: I need to update the test data files in xbout/tests/data/dump_files to add jyseps1_2, etc. to get the tests to pass. The files are ~21MB - I think it might be better to remove the .nc files and replace them with a simple python script/function to generate the required files. Thoughts? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#36?email_source=notifications&email_token=AISNPI3EQXKJFD7ZPIC4E63QPWYGRA5CNFSM4IDYL3E2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB2NAFI#issuecomment-544526357>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AISNPIY22DW3UMWWLXQC5PTQPWYGRANCNFSM4IDYL3EQ> .

[johnomotani]

Do they need to be 21MB?

Not sure, I don't know why they were originally created with the structure they have. But there are 32 files in the tests/data/dump_files/along_x directory, which seems like a lot, so I'd guess they could be cut down.

[TomNicholas]

I think my logic was along the lines of: We want to test concatenation on the most general output, which means having files with all possible combinations of guard cells (so 3 for times 3 for y), then combine along along time (so times 2). So maybe the minimum number of files is 18? But each one doesn't need to have much data in it.

…

On Mon, 21 Oct 2019, 17:06 johnomotani, ***@***.***> wrote: Do they need to be 21MB? Not sure, I don't know why they were originally created with the structure they have. But there are 32 files in the tests/data/dump_files/along_x directory, which seems like a lot, so I'd guess they could be cut down. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#36?email_source=notifications&email_token=AISNPIZT4LBTGM3OQWHKGCTQPXHRHA5CNFSM4IDYL3E2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB23SII#issuecomment-544586017>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AISNPI4SHY2V4QVALG4FOWTQPXHRHANCNFSM4IDYL3EQ> .

[johnomotani]

makes sense, but I'll try and replace by using the create_bout_ds_list function so we can get rid of the binary files. If that works out, it might even be worth deleting the binary files from the history (while there are still not so many users) to make the xBOUT download smaller, I think those files are like 90% of the disk space used.

[johnomotani]

Actually, because create_bout_ds_list uses xarray (and because I mostly wrote the script already), I'm tempted to put in a function written with just numpy and netCDF4.Dataset to create the 'reference' data files. It seems to me like that minimises the dependence on the tools that we want to test, when creating the test data.

[TomNicholas]

I like the create_bout_ds_list function, but I'm unsure why you would ever use netCDF4.Dataset when xarray exists? xarray wraps netCDF, you should be able to accomplish the same thing in fewer lines with one less explicit dependency.

[johnomotani]

My thinking is that we depend strongly on xarray, so maybe it's better not to use xarray to generate test input. netCDF4 is independent and probably more stable. So we're testing xarray as well as xBOUT. Maybe that's the wrong point of view - instead we should trust xarray's tests, and say we're specifically testing xBOUT so it's fine to use xarray to generate input. Any thoughts @ZedThree?

[rdoyle45]

A slight digression gents; on the errors thrown by the checks of a4ad8b7, those in in Python 3.6 have been solved in xarray and will be in the next release as mentioned by @johnomotani earlier. Whereas those thrown by Python 3.5, relating to the path passed to the generate_test_data function, are specific to Python 3.5.

I don't believe this is of the greatest importance, but should more recent releases of python be used for test? Maybe 3.6 and 3.7 instead (with a possible inclusion of 3.8) or is the backwards compatibility of xBOUT to 3.5 of greater importance? @TomNicholas

[rdoyle45]

This should solve the problem with python 3.5 test.

[TomNicholas]

Thanks Rhys. As xBOUT is not even at an initial release version yet I see no reason to strive for backwards compatibility to different versions of python, we can just make 3.6 a requirement.

…

On Tue, 22 Oct 2019 at 12:28, Rhys Doyle ***@***.***> wrote: ***@***.**** requested changes on this pull request. This should solve the problem with python 3.5 test. ------------------------------ In xbout/tests/test_animate.py <#36 (comment)>: > @@ -0,0 +1,41 @@ +import pytest + +from xbout import open_boutdataset +from xbout.boutdataarray import BoutDataArrayAccessor +from .generate_test_data import generate_test_data + +from animatplot.blocks import Imshow, Line + + ***@***.*** +def create_test_file(tmpdir_factory): + + # Create temp dir for output of animate1D/2D + save_dir = tmpdir_factory.mktemp("test_data") ⬇️ Suggested change - save_dir = tmpdir_factory.mktemp("test_data") + save_dir = str(tmpdir_factory.mktemp("test_data")) ------------------------------ In xbout/tests/test_animate.py <#36 (comment)>: > +from xbout.boutdataarray import BoutDataArrayAccessor +from .generate_test_data import generate_test_data + +from animatplot.blocks import Imshow, Line + + ***@***.*** +def create_test_file(tmpdir_factory): + + # Create temp dir for output of animate1D/2D + save_dir = tmpdir_factory.mktemp("test_data") + + # Generate some test data + generate_test_data(save_dir, nt=3, nx=16, ny=6, nz=2, NXPE=3, NYPE=3) + + ds = open_boutdataset(save_dir.join("BOUT.dmp.*.nc")) # Open test data ⬇️ Suggested change - ds = open_boutdataset(save_dir.join("BOUT.dmp.*.nc")) # Open test data + ds = open_boutdataset(save_dir + "/BOUT.dmp.*.nc") # Open test data — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36?email_source=notifications&email_token=AISNPI4TU4MSA5WVIYCSHA3QP3PU5A5CNFSM4IDYL3E2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCIX67ZQ#pullrequestreview-305131494>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AISNPI7N2V2AX7THOSZHEG3QP3PU5ANCNFSM4IDYL3EQ> .

-- Thomas Nicholas PhD Student | Computational plasma physics for nuclear fusion University of York | Based at Culham Centre for Fusion Energy <http://www.ccfe.ac.uk/> (D3/1.79) GitHub <https://github.com/TomNicholas> | LinkedIn <https://www.linkedin.com/in/tom-nicholas/>

[johnomotani]

Thanks @rdoyle45!
Seems like the error in Python-3.5 is a known issue with pathlib and pytest, which is resolved in Python-3.6.
https://stackoverflow.com/questions/40784950/pathlib-path-and-py-test-localpath
As pathib.join() is nicer than converting to string (should be more cross-platform compatible), I'd agree that it's best to just require Python-3.6.

[rdoyle45]

Agreed on both counts. @johnomotani

[johnomotani]

Discussion on Slack agreed with @TomNicholas, so I've removed the generate_test_data() function that used netCDF4.Dataset, and instead used test_load.create_bout_ds_list(). I've also extended create_bout_ds/create_bout_ds_list a bit to create files that are more similar to BOUT++ output.

outdated

[TomNicholas]

This is great - thanks both of you.