New attempt for HDF5 support using HighFive

[tdegeus]

Hereby my attempt for a cleaner HighFive support. It allows the following syntax:

#include <xtensor-io/xhighfive.hpp>

int main()
{
  HighFive::File file("example.h5", HighFive::File::Overwrite);

  xt::xtensor<double,2> A = xt::ones<double>({10,5});

  xt::dump(file, "/path/to/data", A);

  xt::xtensor<double,2> B = xt::cast<double,2>(file, "/path/to/data");

  return 0;
}

What do you guys think?

Note that:

• I still want to add some overwrite function to overwrite an existing data-set (of the correct shape/type) with new data.

• Ideally I want to add some user friendliness, like having dump also work for example for std::string, that I miss in HighFive, but I could also construct my own wrapper for that.

• I believe that the recursive createGroup function should really be in the HighFive library. I have openend an issue there: Recursively create groups BlueBrain/HighFive#139

[tdegeus]

P.S. As I missed some functions to smoothen HighFive, I have made some wrapper functions:

• https://github.com/tdegeus/HDF5pp/blob/master/include/LowFive.h
• http://hdf5pp.geus.me/en/latest/LowFive.html

[wolfv]

do you want to integrate these functions in xtensor-io or rather upstream in HighFive?

Nice name, btw!

[tdegeus]

Well I don't know.

On first sight, I find the one-line syntax very much in the line of xtensor, and a bit less with HighFive, as HighFive appears to focus on keeping the full flexibility of HDF5. On the other hand, I believe that h5py features both. In any case, I find the one-line syntax vital to get readable main-files. So I strongly believe that such syntax should be available.

Maybe you can share your thoughts?

Note that the namespaces and the syntax is not in set in stone, neither of the PR nor of LowFive. We can discuss about everything.

[tdegeus]

I have upgraded the PR significantly. I will carefully review the code again tomorrow or so, but it would be great to have general feedback. The syntax is now:

#include <xtensor-io/xhighfive.hpp>

int main()
{
  HighFive::File file("example.h5", HighFive::File::Overwrite);

  xt::xtensor<double,2> A = xt::ones<double>({10,5});

  xt::dump(file, "/path/to/data", A);

  xt::overwrite(file, "/path/to/data", A);

  xt::xtensor<double,2> B = xt::load<xt::xtensor<double,2>>(file, "/path/to/data");

  return 0;
}

whereby dump, overwrite, and load feature many overloads.

As discussed with @wolfv in private, I will also submit these functions to be integrated upstream. They will stay also in xtensor-io to:

1. Have full control over rapid upgrades.
2. Allow for a unified syntax.
3. Allow more dedicated xtensor support. Therefore your help is wanted! dump could feature more overloads, such as that it would be awesome to be able to write xt::views.

The more dedicated xtensor support will be exclusive to xtensor-io. I will only try to integrate basic xt::xtensor and xt::xarray support.

Beyond code review, to-do are:

1. Documentation.
2. (To be discussed) Tests. Here help is wanted, in so far to handle the HighFive header-only library and linking against hdf5 in CI.

[wolfv]

Regarding testing: We ship + download all dependencies on conda. I can make a HighFive package for conda and we can first put it on the quantstack channel, but in the long term it should go to conda-forge !

[JohanMabille]

Nice PR!

Regarding the interface, as discussed with @wolfv, and to summarize what have been told on gitter channel, there should be a clean separation between pure xtensor API and lower level API making use of HighFive:

namespace xt
{
    // Not specific to HDF5, can be used by functions writing other file formats,
    // so that should be in a separate header
    enum class dump_mode
    {
        create,
        overwrite
    }

    template <class T>
    void dump_hdf5(const std::string& file_path,
                   const std::string& data_path, 
                   const T& data,
                   dump_mode mode);

    template <class T>
    T load_hdf5(cons std::string& file_path, const std::string& data_path);
}

These functions forward to the dump / overwrite / load functions that accept an HighFive::File format that you have coded. @wolfv suggested these functions could be in their own namespace (xt::hdf5) to reinforce the separation between pure xtensor API and mixed API, I am not really opiniated about that.

[tdegeus]

The dump_hdf5 function should have one more argument I think: the mode with which to open the file. What would be the desired syntax for that?

I'm not strongly opinionated about the xt::hdf5 syntax neither, though I don't think you win anything as the HighFive functions are always distinguishable through their arguments. If you introduce a namespace though I feel that it should be HighFive as the primary interaction is through that library.

@JohanMabille @wolf

[JohanMabille]

The dump_hdf5 function should have one more argument I think: the mode with which to open the file. What would be the desired syntax for that?

You mean so one can replace the entire file and not only the path to the data inside? That's what dump_mode is supposed to do, I forgot the mode to overwrite the data inside. That can be another enum specific to hdf5 (like hdf5_mode with the same values as the ones of dump_mode).

[tdegeus]

@JohanMabille Overwriting a DataSet may not be specific to HDF5, e.g. the same operation is perceivable for an NPZ-file (though I'm not an expert, so I don't know if it is actually possible). So I would argue for

• file_mode: how to open the file.
• dump_mode: how to handle existing DataSet.

[JohanMabille]

Sounds perfect for me.

[tdegeus]

@JohanMabille @wolfv

Please find as I believe all discussed updates + documentation + tests.

I do still need help to complete docs/source/api_reference.rst with all the overloads.

[wolfv]

Did you already add highfive to the conda install directive in .travis.yml?

We could try to make tests pass on Travis/Linux by adding highfive here: https://github.com/QuantStack/xtensor-io/blob/master/.travis.yml#L39

[wolfv]

Ok, now i am hopeful that the tests will be passed.

[wolfv]

Great stuff in this PR :)

Some nitpicking follows:

For function arguments we usually write references like so: HighFive::File& file (e.g. & right after type, then space).
We put spaces around operators (e.g. i + j)

And I just asked for a bit of clarification with the scalar load interface.

Let's merge this soon!

[wolfv]

btw feel free to squash my commits

[tdegeus]

I could also squash them all right?

…

On 22 Nov 2018, at 14:13, Wolf Vollprecht ***@***.***> wrote: btw feel free to squash my commits — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGtGrVwGeEcTF4pw1_Zt29x1xz3y5EJhks5uxqLlgaJpZM4YXWtK>.

[wolfv]

yep, that would be nice. Or at least reduce the number of commits a bit so we have a clean history.

[wolfv]

Yep, if there is only one thats enough . But for the `dump` methods, shouldn't they go here as well? Le 23 nov. 2018 10:19, "Tom de Geus" <notifications@github.com> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In docs/source/api_reference.rst <#41 (comment)>: > @@ -21,6 +21,17 @@ Defined in ``xtensor-io/xnpz.hpp`` .. doxygenfunction:: xt::dump_npz :project: xtensor-io +HDF5 files +---------- + +Defined in ``xtensor-io/xhighfive.hpp`` + +.. doxygenfunction:: xt::dump_hdf5 + :project: xtensor-io + +.. doxygenfunction:: xt::load_hdf5 + :project: xtensor-io + Actually, there is only one signature with an all accepting template. So this should be correct? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2BPuc8PCNs-f1OB0hYfo4pAlccagwvks5ux72IgaJpZM4YXWtK> .

[tdegeus]

I'll push a complete version for you to double check. I think I have everything now.

[tdegeus]

It's there for you to check!

[tdegeus]

@wolfv Maybe you can add a conda install/compile strategy to the documentation?

[wolfv]

Let's merge for now!

I'll try to find some time next week to make this interface also take an xexpression (as discussed).

[wolfv]

And then we should cut a new release soon

[tdegeus]

Awesome!