Fix some HDF5 related issues and add Mesh::getLocal{X,Y,Z}Index #2102

ZedThree · 2020-09-21T17:04:43Z

Fixes #2003

Also:

Fix bug in DataFormat::writeFieldAttributes with FieldPerp: need to use the global index that doesn't include boundaries
Fix test-io_hdf5 needing global mesh/dump
Build with HDF5 on Travis
Require netCDF for some tests that use grid files
Add Mesh::getLocal{X,Y,Z}Index{NoBoundaries} methods: these are inverses of the getGlobal versions, replacing {X,Y}LOCAL, and are necessary for consistent, unique global indices into the boundaries for double-null tokamak grids

There are some tests that can't use HDF5 because they use grid files, and I don't think we want to have duplicate binaries just in different formats. There are some other tests that could use HDF5 but currently can't because we'd need to be able to change the file extension in the runtest script. And some others which should be run with both HDF5 and netCDF, including some duplicated ones that could be merged, if we could query what formats are available in runtest. Not sure how much I actually want to do there, I'm not terribly certain we want to support HDF5 into the future.

Fedora test is failing because it needs h5py, but I didn't know where to specify that -- @dschwoerer does it need to be in your BOUT++ Fedora recipe? If so, is it possible to give me maintainer access to that?

Some of this should be backported.

`H5Awrite` and `H5Aread` actually need `(const) char**`: I ended up just looking at the source for the C++ API to see how they do it themselves

Writing `yindex_global` was not the inverse of reading it

- Apply black formatting - Suppress warning from `rm` - Don't quit on first failure - Fix typo ("Field3D" -> "Field2D") - Use `numpy.allclose`

Some tests use grid files which puts a hard dependency on netCDF, but others could be adapted to use either netCDF or HDF5 if we had a nice way to determine which is available at test-time (and maybe run the test for both if needed)

src/fileio/dataformat.cxx

dschwoerer · 2020-09-21T20:32:58Z

And some others which should be run with both HDF5 and netCDF, including some duplicated ones that could be merged, if we could query what formats are available in runtest.

The boutconf branch might help with that, as that lets you query how bout was configured.

Not sure how much I actually want to do there, I'm not terribly certain we want to support HDF5 into the future.

I'd be in favor to only support one backend - makes it easier to maintain and test. Or what is the advantage of HDF5? Slighly easier to install compared to netcdf?

Fedora test is failing because it needs h5py, but I didn't know where to specify that -- @dschwoerer does it need to be in your BOUT++ Fedora recipe? If so, is it possible to give me maintainer access to that?

I added it to the install line. I wouldn't mind giving you access to the package in fedora, but that requires you to be a packager, which involves demonstrating you know the packaging rules of fedora. I certainly learned a lot in the process and it is fun, but also requires a bit of time ...

johnomotani · 2020-09-21T20:42:48Z

I don't really remember (maybe @bendudson does?), but I think there was a thought at one point that some project (maybe the American SciDAC?) would standardise on HDF5, and it would be useful to be able to integrate with that. I happened to be working on an HDF5 interface for another project a few years ago, so thought I might as well put in a BOUT++ one while I was looking up the function calls.

I'd support standardising to one backend. netCDF seems better in my opinion, the ecosystem of tools around xarray, etc. all seems to be built mostly on netCDF. There shouldn't be any performance difference because netCDF is just HDF5 with extra conventions for metadata.

I would like to get rid of the netcdf-cxx4 dependency though if we're refactoring the backends. It always seems to be a bit of a pain to get onto clusters, and in my experience isn't provided in the modules.

These are inverses of the `getGlobal` versions, replacing `{X,Y}LOCAL`, and are necessary for consistent, unique global indices into the boundaries for double-null tokamak grids

include/bout/mesh.hxx

src/fileio/dataformat.cxx

johnomotani · 2020-09-22T18:11:29Z

src/fileio/dataformat.cxx

  } else {
-    f.setIndex(f.getMesh()->YLOCAL(0));
+    f.setIndex(f.getMesh()->getLocalYIndexNoBoundaries(0));


I think it makes sense to keep getLocalYIndexNoBoundaries(0) here, as that sets a default value on a grid cell rather than (possibly) in a boundary cell.

I'm not sure about this, might be better to just fail?

I wanted to not fail so we can successfully read old restart files, which might (I think? haven't checked the actual history...) exist with FieldPerps but without yindex_global attributes.

That sounds reasonable, I'm just not sure how you'd deal with such a field? You'd need to set the index via some other means

src/mesh/impls/bout/boutmesh.cxx

include/bout/mesh.hxx

Co-authored-by: johnomotani <john.omotani@ukaea.uk>

Use boundary-aware form of global index conversion, but check it's an interior point when writing it.

johnomotani · 2020-09-23T20:43:21Z

I think this needs #2095 for the tests to pass.

BTW, I tested with the netcdf-legacy and the tests pass, but fperp2 is not read or written on the correct processors! The legacy netcdf interface just fails to read attributes from the benchmark file. Maybe because it's a NetCDF4-format and legacy netcdf doesn't support NetCDF4 properly? I changed the runtest scripts in #2095, so I'll add a check of the attributes in that PR.

johnomotani

Other than a merge of #2095, this looks good to me.

dschwoerer · 2020-09-24T06:29:37Z

I guess we should drop legacy netcdf and hdf5 with the 5.0 release.
I think it is better to fail early and with a clear error, then to try to do the wrong thing, but produce slightly wrong results.

ZedThree · 2020-09-24T10:32:31Z

It looks like this should just go into #2095 ?

I guess we should drop legacy netcdf and hdf5 with the 5.0 release.

After we get 4.3.2 and 4.4.0 out, I want to look at completely replacing datafile with OptionsNetCDF

johnomotani · 2020-09-24T10:47:03Z

It looks like this should just go into #2095 ?

That sounds sensible.

src/fileio/dataformat.cxx

To be more precise, mark the global yindex for FieldPerps in guard cells as invalid when writing them, and set the (local) yindex to be invalid if its in a guard cell on this processor when reading them. It is still possible to write FieldPerps in guard cells, it's just that `collect` won't read them, and the user will need to use a lower-level API to do so.

Also: - Don't plot anything on failure by default - Clean up quietly - More consistent pass/fail message

ZedThree added 9 commits September 21, 2020 11:20

Fix reading/writing std::string attributes when using HDF5

342b145

`H5Awrite` and `H5Aread` actually need `(const) char**`: I ended up just looking at the source for the C++ API to see how they do it themselves

Fix bug in DataFormat::writeFieldAttributes with FieldPerp

de6f387

Writing `yindex_global` was not the inverse of reading it

Enable some HDF5-specific tests in default test set

6308b89

Enable HDF5 in all Travis jobs

7b8e2ae

Some tidying up to test-restarting

359c95a

- Apply black formatting - Suppress warning from `rm` - Don't quit on first failure - Fix typo ("Field3D" -> "Field2D") - Use `numpy.allclose`

Use global mesh/dump in test-io_hdf5

5babe4f

Require netCDF for some tests

1bbce72

Some tests use grid files which puts a hard dependency on netCDF, but others could be adapted to use either netCDF or HDF5 if we had a nice way to determine which is available at test-time (and maybe run the test for both if needed)

CMake: Add missing requirement on PETSc for AMG test

9f03122

Remove unnecessary hardcoding of dump_format in test-invpar

e926569

ZedThree added bugfix backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 labels Sep 21, 2020

ZedThree requested review from johnomotani and dschwoerer September 21, 2020 17:04

johnomotani requested changes Sep 21, 2020

View reviewed changes

src/fileio/dataformat.cxx Outdated Show resolved Hide resolved

install python3-h5py on travis@fedora

9023cfd

dschwoerer force-pushed the fix-hdf5-restart branch from 975343b to 9023cfd Compare September 22, 2020 05:59

ZedThree added 2 commits September 22, 2020 11:44

Add Mesh::getLocal{X,Y,Z}Index{NoBoundaries} methods

0f971d1

These are inverses of the `getGlobal` versions, replacing `{X,Y}LOCAL`, and are necessary for consistent, unique global indices into the boundaries for double-null tokamak grids

Use more consistent global index when writing/reading FieldPerps

b26b4d9

ZedThree changed the title ~~Fix some HDF5 related issues~~ Fix some HDF5 related issues and add Mesh::getLocal{X,Y,Z}Index Sep 22, 2020

ZedThree requested a review from johnomotani September 22, 2020 10:46

ZedThree and others added 2 commits September 22, 2020 15:21

Use no boundary form of global index conversion in FieldPerp I/O

3539dbb

Remove deprecated YLOCAL() in examples/laplace-petsc3d

4568179

johnomotani requested changes Sep 22, 2020

View reviewed changes

ZedThree and others added 2 commits September 23, 2020 16:23

Rename *local -> *global argument in mesh index routines

ed4cc62

Co-authored-by: johnomotani <john.omotani@ukaea.uk>

Better fix for reading/writing FieldPerp index

e48774f

Use boundary-aware form of global index conversion, but check it's an interior point when writing it.

ZedThree mentioned this pull request Sep 23, 2020

Bugfix release: v4.3.2 #2104

Merged

johnomotani mentioned this pull request Sep 23, 2020

Handle FieldPerps in Datafile::varAdded() and Datafile::varPtr() (next) #2095

Merged

johnomotani previously approved these changes Sep 23, 2020

View reviewed changes

ZedThree commented Sep 24, 2020

View reviewed changes

src/fileio/dataformat.cxx Outdated Show resolved Hide resolved

ZedThree added 3 commits September 25, 2020 11:03

Use last boundary point for global y-index in restart IO test

d1e3fdc

Make sure test-restart-io sets exit code correctly on failure

48c4cbb

Also: - Don't plot anything on failure by default - Clean up quietly - More consistent pass/fail message

ZedThree dismissed johnomotani’s stale review via 48c4cbb September 25, 2020 10:34

johnomotani approved these changes Sep 25, 2020

View reviewed changes

johnomotani mentioned this pull request Sep 25, 2020

Loading FieldPerp variables boutproject/xBOUT#148

Open

ZedThree merged commit 5461a40 into next Sep 25, 2020

ZedThree deleted the fix-hdf5-restart branch September 25, 2020 12:53

ZedThree mentioned this pull request Oct 5, 2020

Fix reading/writing std::string attributes when using HDF5 #2116

Merged

ZedThree mentioned this pull request Nov 24, 2020

test-restarting fails without netcdf #2003

Closed

This was referenced Aug 4, 2021

Release V4.4.0 #2383

Merged

Fix some HDF5 related issues and add Mesh::getLocal{X,Y,Z}Index (v4.4) #2394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some HDF5 related issues and add Mesh::getLocal{X,Y,Z}Index #2102

Fix some HDF5 related issues and add Mesh::getLocal{X,Y,Z}Index #2102

ZedThree commented Sep 21, 2020 •

edited

dschwoerer commented Sep 21, 2020

johnomotani commented Sep 21, 2020

johnomotani Sep 22, 2020

ZedThree Sep 23, 2020

johnomotani Sep 23, 2020

ZedThree Sep 24, 2020

johnomotani commented Sep 23, 2020

johnomotani left a comment

dschwoerer commented Sep 24, 2020

ZedThree commented Sep 24, 2020

johnomotani commented Sep 24, 2020

Fix some HDF5 related issues and add Mesh::getLocal{X,Y,Z}Index #2102

Fix some HDF5 related issues and add Mesh::getLocal{X,Y,Z}Index #2102

Conversation

ZedThree commented Sep 21, 2020 • edited

dschwoerer commented Sep 21, 2020

johnomotani commented Sep 21, 2020

johnomotani Sep 22, 2020

Choose a reason for hiding this comment

ZedThree Sep 23, 2020

Choose a reason for hiding this comment

johnomotani Sep 23, 2020

Choose a reason for hiding this comment

ZedThree Sep 24, 2020

Choose a reason for hiding this comment

johnomotani commented Sep 23, 2020

johnomotani left a comment

Choose a reason for hiding this comment

dschwoerer commented Sep 24, 2020

ZedThree commented Sep 24, 2020

johnomotani commented Sep 24, 2020

ZedThree commented Sep 21, 2020 •

edited