An example using hdf5 references (#396) #397

plusangel · 2021-01-16T10:16:14Z

I included an example to demonstrate how references work in HighFive. PR suggested by @tdegeus

Thank you guys

tdegeus

I'm probably not the best person to review, because I don't actually know references in HDF5. That being said, it didn't become clear from this example either... Some line of text would surely help there: describe what the example is doing in a few lines of comment somewhere.

Where I really got lost though is with the std::vector<Reference>. The question is: Are reference in HDF5 something similar to those in C++? If so, wouldn't it be easier to (also) have a basic example with one reference?

tdegeus · 2021-01-19T08:41:57Z

src/examples/read_write_vector_dataset_references.cpp

+const std::string FILE_NAME("dataset_integer.h5");
+const std::string SOURCE_INT_DATASET_NAME("source_dateset");
+const std::string REFERENCE_DATASET_NAME("reference_dateset");


Personally I'm not a fan of these 'defines' (although I know that it's done in other examples). I'm not sure if it helps understanding the example. So I would probably use strings as argument below, especially when the string is used only once.

Regardless, it would be more consistent to use 'defines' everywhere or nowhere. I.e. if it is kept like this, the group name should also be defined this way.

Also I would propose that you use dataset not dateset ;)

I don't mind either. Defining the strings doesn't hurt, but doesn't add much. Either is fine.

tdegeus · 2021-01-19T08:42:08Z

src/examples/read_write_vector_dataset_references.cpp

+const std::string FILE_NAME("dataset_integer.h5");
+const std::string SOURCE_INT_DATASET_NAME("source_dateset");
+const std::string REFERENCE_DATASET_NAME("reference_dateset");
+const size_t size_dataset = 20;


Same here, does it really make the example more comprehensible to define it separately here?

tdegeus · 2021-01-19T08:42:43Z

src/examples/read_write_vector_dataset_references.cpp

+const std::string REFERENCE_DATASET_NAME("reference_dateset");
+const size_t size_dataset = 20;
+
+// create a dataset 1D from a vector of string


I don't think that that is true, you specify a vector of ints

tdegeus · 2021-01-19T08:43:02Z

src/examples/read_write_vector_dataset_references.cpp

+    File file(FILE_NAME, File::ReadWrite | File::Create | File::Truncate);
+
+    // we create a new group
+    Group group = file.createGroup("a_group");


"mygroup"?

why do you define const strings for the file name and dataset names, but not the group. I would be consistent, whatever you choose.

tdegeus · 2021-01-19T08:44:27Z

src/examples/read_write_vector_dataset_references.cpp

+    // let's create a dataset of native integer with the size of the vector
+    // 'data' inside the group


I don't think that this is true either. You actually write the data too, you are not only allocating some 'empty' vector

src/examples/read_write_vector_dataset_references.cpp

tdegeus · 2021-01-19T08:48:21Z

src/examples/read_write_vector_dataset_references.cpp

+#include <highfive/H5File.hpp>
+#include <highfive/H5Reference.hpp>
+
+using namespace HighFive;


I know that this is done in other examples too. However, personally I find this an absolute no-go. If one is new to the library (and/or relatively new to C++) this will create nothing but confusion. If it about the line-length one can always alias from something short.

Probably the others will argue that one should stay consistent with other examples.

I agree, please remove the using namespace declaration. As @tdegeus said, this just makes things less clear and is generally a bad practice that should be avoided.

ohm314

Hi @plusangel, sorry for taking so long to review this. Overall the example is small enough and a good idea to add. We actually have in the unit tests already something very similar (but I understand that looking at test cases is a pain, especially when you're just getting into the library).
@tdegeus gave already some very good feedback. Could I ask you to please implement his and my suggestions into an additional patch. I"ll try to then re-review fairly quickly and have this merged!
Here's the test case, for reference: https://github.com/BlueBrain/HighFive/blob/master/tests/unit/tests_high_five_base.cpp#L1587

ohm314 · 2021-03-17T21:30:53Z

src/examples/read_write_vector_dataset_references.cpp

+#include <highfive/H5File.hpp>
+#include <highfive/H5Reference.hpp>
+
+using namespace HighFive;


I agree, please remove the using namespace declaration. As @tdegeus said, this just makes things less clear and is generally a bad practice that should be avoided.

ohm314 · 2021-03-17T21:32:03Z

src/examples/read_write_vector_dataset_references.cpp

+using namespace HighFive;
+
+const std::string FILE_NAME("dataset_integer.h5");
+const std::string SOURCE_INT_DATASET_NAME("source_dateset");


Suggested change

const std::string SOURCE_INT_DATASET_NAME("source_dateset");

const std::string SOURCE_INT_DATASET_NAME("source_dataset");

ohm314 · 2021-03-17T21:32:46Z

src/examples/read_write_vector_dataset_references.cpp

+const std::string FILE_NAME("dataset_integer.h5");
+const std::string SOURCE_INT_DATASET_NAME("source_dateset");
+const std::string REFERENCE_DATASET_NAME("reference_dateset");


I don't mind either. Defining the strings doesn't hurt, but doesn't add much. Either is fine.

ohm314 · 2021-03-17T21:33:30Z

src/examples/read_write_vector_dataset_references.cpp

+const std::string REFERENCE_DATASET_NAME("reference_dateset");
+const size_t size_dataset = 20;
+
+// create a dataset 1D from a vector of string


Suggested change

// create a dataset 1D from a vector of string

// create a dataset 1D from a vector of int

ohm314 · 2021-03-17T21:34:44Z

src/examples/read_write_vector_dataset_references.cpp

+    File file(FILE_NAME, File::ReadWrite | File::Create | File::Truncate);
+
+    // we create a new group
+    Group group = file.createGroup("a_group");


why do you define const strings for the file name and dataset names, but not the group. I would be consistent, whatever you choose.

ohm314 · 2021-03-17T21:40:32Z

src/examples/read_write_vector_dataset_references.cpp

+    Reference ref = Reference(group, dataset);
+    std::vector<Reference> ref_container{ref};
+
+    // in similar fashion, we store as dataset the reference that we want


Suggested change

// in similar fashion, we store as dataset the reference that we want

// in similar fashion, we store as dataset the vector of references that we want

…tdegeus feedback

plusangel · 2021-03-19T17:48:43Z

Thank you both for your valuable comments and your time to review it. I think I catch them all. Keep up the good work. Thank you!

ferdonline · 2021-03-25T22:19:49Z

src/examples/read_write_vector_dataset_references.cpp

@@ -0,0 +1,97 @@
+/*
+ *  Copyright (c), 2017, Adrien Devresse


Suggested change

* Copyright (c), 2017, Adrien Devresse

* Copyright (c), 2021 Blue Brain Project - EPFL

ferdonline · 2021-03-25T22:20:54Z

src/examples/read_write_vector_dataset_references.cpp

+// const std::string FILE_NAME("dataset_integer.h5");
+//const std::string SOURCE_INT_DATASET_NAME("source_dateset");
+//const std::string REFERENCE_DATASET_NAME("reference_dataset");
+//const size_t size_dataset = 20;
+


Suggested change

// const std::string FILE_NAME("dataset_integer.h5");

//const std::string SOURCE_INT_DATASET_NAME("source_dateset");

//const std::string REFERENCE_DATASET_NAME("reference_dataset");

//const size_t size_dataset = 20;

I personally liked the definitions in the header, but if you don't use them delete the code

ferdonline · 2021-03-25T22:33:44Z

src/examples/read_write_vector_dataset_references.cpp

+    HighFive::File file("dataset_integer.h5", HighFive::File::ReadWrite |
+                                                  HighFive::File::Create |
+                                                  HighFive::File::Truncate);


Suggested change

HighFive::File file("dataset_integer.h5", HighFive::File::ReadWrite |

HighFive::File::Create |

HighFive::File::Truncate);

HighFive::File file("dataset_integer.h5", HighFive::File::Overwrite);

ferdonline · 2021-03-25T22:39:50Z

src/examples/read_write_vector_dataset_references.cpp

+    std::vector<int> data(20);
+    for (size_t i = 0; i < data.size(); ++i) {
+        data[i] = int(i);
+    }


Suggested change

std::vector<int> data(20);

for (size_t i = 0; i < data.size(); ++i) {

data[i] = int(i);

}

std::vector<int> data(20);

std::iota(data.begin(), data.end(), 0);

ferdonline · 2021-03-25T22:42:50Z

src/examples/read_write_vector_dataset_references.cpp

+    HighFive::DataSet dataset = group.createDataSet<int>("source_dateset",
+                                                         HighFive::DataSpace::From(data));
+    dataset.write(data);


Suggested change

HighFive::DataSet dataset = group.createDataSet<int>("source_dateset",

HighFive::DataSpace::From(data));

dataset.write(data);

auto dataset = group.createDataSet("source_dataset", data);

ferdonline · 2021-03-25T22:44:10Z

src/examples/read_write_vector_dataset_references.cpp

+    }
+
+    return 0;  // successfully terminated
+}


Suggested change

}

}

ferdonline · 2021-03-25T22:48:50Z

src/examples/read_write_vector_dataset_references.cpp

+    HighFive::DataSet ref_set = group.createDataSet<HighFive::Reference>(
+        "reference_dataset", HighFive::DataSpace::From(ref_container));
+    ref_set.write(ref_container);


Suggested change

HighFive::DataSet ref_set = group.createDataSet<HighFive::Reference>(

"reference_dataset", HighFive::DataSpace::From(ref_container));

ref_set.write(ref_container);

HighFive::DataSet ref_set = group.createDataSet("reference_dataset", ref_container);

ferdonline · 2021-03-25T22:54:42Z

src/examples/read_write_vector_dataset_references.cpp

+        write_dataset();
+        read_dataset();
+
+    } catch (HighFive::Exception& err) {


Suggested change

} catch (HighFive::Exception& err) {

} catch (const HighFive::Exception& err) {

ferdonline · 2021-03-31T12:08:34Z

@plusangel Do you have an estimate on when you could address this PR suggestions?

…feedback

plusangel · 2021-03-31T15:03:56Z

Sorry for the delay @ferdonline. Done :)

ohm314

lgtm

An example using hdf5 references

1a76fbd

pramodk requested review from alkino and ferdonline January 16, 2021 14:47

tdegeus reviewed Jan 26, 2021

View reviewed changes

ohm314 self-requested a review March 17, 2021 12:53

ohm314 suggested changes Mar 17, 2021

View reviewed changes

The example of using hdf5 references was updated based on ohm314 and …

43154b4

…tdegeus feedback

ferdonline requested changes Mar 25, 2021

View reviewed changes

The example of using hdf5 references was updated based on @ferdonline …

87c81d5

…feedback

ferdonline previously approved these changes Mar 31, 2021

View reviewed changes

ferdonline requested a review from ohm314 March 31, 2021 16:01

Update src/examples/read_write_vector_dataset_references.cpp

e113e75

tdegeus dismissed ferdonline’s stale review via e113e75 March 31, 2021 16:04

tdegeus approved these changes Mar 31, 2021

View reviewed changes

ohm314 approved these changes Mar 31, 2021

View reviewed changes

ferdonline merged commit 1841062 into BlueBrain:master Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

An example using hdf5 references (#396) #397

An example using hdf5 references (#396) #397

plusangel commented Jan 16, 2021

tdegeus left a comment

tdegeus Jan 19, 2021

ohm314 Mar 17, 2021

tdegeus Jan 19, 2021

tdegeus Jan 19, 2021

tdegeus Jan 19, 2021

ohm314 Mar 17, 2021

tdegeus Jan 19, 2021

tdegeus Jan 19, 2021

ohm314 Mar 17, 2021

ohm314 left a comment

ohm314 Mar 17, 2021

ohm314 Mar 17, 2021

ohm314 Mar 17, 2021

ohm314 Mar 17, 2021

ohm314 Mar 17, 2021

ohm314 Mar 17, 2021

plusangel commented Mar 19, 2021

ferdonline Mar 25, 2021

ferdonline Mar 25, 2021

ferdonline Mar 25, 2021

ferdonline Mar 25, 2021

ferdonline Mar 25, 2021

ferdonline Mar 25, 2021

ferdonline Mar 25, 2021

ferdonline Mar 25, 2021

ferdonline Mar 25, 2021

ferdonline commented Mar 31, 2021

plusangel commented Mar 31, 2021

ohm314 left a comment

		// let's create a dataset of native integer with the size of the vector
		// 'data' inside the group

	const std::string SOURCE_INT_DATASET_NAME("source_dateset");
	const std::string SOURCE_INT_DATASET_NAME("source_dataset");

	// create a dataset 1D from a vector of string
	// create a dataset 1D from a vector of int

	// in similar fashion, we store as dataset the reference that we want
	// in similar fashion, we store as dataset the vector of references that we want

	* Copyright (c), 2017, Adrien Devresse
	* Copyright (c), 2021 Blue Brain Project - EPFL

	} catch (HighFive::Exception& err) {
	} catch (const HighFive::Exception& err) {

An example using hdf5 references (#396) #397

An example using hdf5 references (#396) #397

Conversation

plusangel commented Jan 16, 2021

tdegeus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohm314 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plusangel commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferdonline commented Mar 31, 2021

plusangel commented Mar 31, 2021

ohm314 left a comment

Choose a reason for hiding this comment