Intermediate stage of reading heat flux values from ASCOT5 HDF5 file

[bielsnohr]

Summary

A number of methods have been added to interrogate an HDF5 file produced by an ASCOT5 run and extract the particle energies and the mesh elements that the particles have hit. These are important steps towards getting heat flux values on the tokamak mesh. All functionality has been added in the AscotProblem class. These will be called from AscotProblem::syncSolutions(), and most of the framework is there now.

Objective

C++ is not my primary language, and I have limited experience with the OO paradigm. It would be good to get some comments around these aspects. In particular:

1. In a somewhat functional programming style, I use quantities returned by methods for further operations. Good example is getActiveEndState() where I am returning an HDF5 Group object which has been created within the method. Naively, there must be overhead for the copy operation since this object won't have scope outside of the method, but from my reading online, lots of compilers handle this this days through Return Value Optimisation (RVO). Some thoughts about whether this is good design would be appreciated.
2. The HDF5 C++ API isn't the most intuitive. Have I made things more complicated than they need to be? Is a shift to the C API advisable?
3. Should I use a try construct when opening the HDF5 file?

[dynamicist]

Answers to specific questions:

"In a somewhat functional programming style, I use quantities returned by methods for further operations. Good example is getActiveEndState() where I am returning an HDF5 Group object which has been created within the method. Naively, there must be overhead for the copy operation since this object won't have scope outside of the method, but from my reading online, lots of compilers handle this this days through Return Value Optimisation (RVO). Some thoughts about whether this is good design would be appreciated."

• Even if RVO/NRVO doesn't happen std::move will be called (from C++11 onwards at least I think, with 1 possible exception prior to C++20) which will avoid the temporary copy
• You could explicitly call std::move on the return value, however std::move will disable NRVO if the compiler implements it. Pretty much all compiles will optimise the code to use std::move anyway, so there's no benefit to relying on the compiler optimization here, which will use RVO if it exists, and std::move if it doesn't.
• The only exception is if you return an rvalue reference and you are using a language standard prior to C++20, in which case it isn't eligible for NRVO and you will need to call std::move explicitly
• In terms of good design, it looks fine to me (there's a reason there are lots of compiler optimisations for it!) and is commonly done.

"The HDF5 C++ API isn't the most intuitive. Have I made things more complicated than they need to be? Is a shift to the C API advisable?"

• If you shift to the C API you'd have to write your own wrapper for the HDF5 C functions, so probably the best approach is to use the existing C++ API (which is a just a wrapper to the C API anyway) unless there's a good reason not to. The code doesn't seem overly complicated to me, and looking at the C-C++ API table I think the C++ wrapper is more intuitive from an OO mindset than the C API.

"Should I use a try construct when opening the HDF5 file?"

• the HDF5 C++ API seems to say that these functions can throw exceptions, so yes you should have a try...catch block to catch them... Unless you are working under the assumption that the caller of your library will deal with any exceptions... which may be the case here as some of the other functions throw MooseExceptions? So maybe catch the exception from the HDF5 api and raise a MooseException instead.

And some general comments:

• I'd avoid using .C for c++ files just in case anyone ever wants to use the code on windows (which is case insensitive for filenames) or with a compiler other than GNU C++. *.cpp is the most portable extension, followed by .cxx and .cc (and a lot of people will use .hpp for c++ headers to indicate the language, but that won't bother the compilers).
• I think getActiveEndstate, getWallTileHits, getParticleEnergies and calculateRelativisticEnergy can all be static functions at present. However, I'm not sure if that's the best design - maybe it would be better if the H5file was a class member variable rather than passed as an argument to the functions. Similarly with the ascot5_active_endstate Group variable.
• Also, are the getActiveEndstate, getWallTileHits, getParticleEnergies functions intended to be called from outside the class (other than in testing)? If not then they should be private... or alternatively you can have another class that reads HDF5 files with more general versions of these functions as public members that read data from the HDF5 file, and test those, rather than expose them as public functions in the AscotProblem class for the purposes of testing only.

Hopefully that all makes sense!

[bielsnohr]

And some general comments:

* I'd avoid using .C for c++ files just in case anyone ever wants to use the code on windows (which is case insensitive for filenames) or with a compiler other than GNU C++.  *.cpp is the most portable extension, followed by .cxx and .cc (and a lot of people will use .hpp for c++ headers to indicate the language, but that won't bother the compilers).

I completely agree, but unfortunately this what the MOOSE framework uses. I am hesitant to change to .cpp in case it breaks things.

* I think getActiveEndstate, getWallTileHits, getParticleEnergies and calculateRelativisticEnergy can all be static functions at present. However, I'm not sure if that's the best design - maybe it would be better if the H5file was a class member variable rather than passed as an argument to the functions. Similarly with the ascot5_active_endstate Group variable.

This will be resolved by suggestion below about moving these routines into a separate class.

* Also, are the getActiveEndstate, getWallTileHits, getParticleEnergies functions intended to be called from outside the class (other than in testing)? If not then they should be private... or alternatively you can have another class that reads HDF5 files with more general versions of these functions as public members that read data from the HDF5 file, and test those, rather than expose them as public functions in the AscotProblem class for the purposes of testing only.

Agreed.