Add test for attributes#77
Conversation
jblomer
left a comment
jblomer
left a comment
There was a problem hiding this comment.
In principle looks good to me. Since we are at it, I think we should make the test slightly more interesting and write three entries, with one attribute attached to the first entry and another attribute attached to the second and third entry.
And then, for a corner case, also test an attribute for an empty RNTuple.
| #include <ROOT/RNTupleModel.hxx> | ||
| #include <ROOT/RNTupleWriteOptions.hxx> | ||
| #include <ROOT/RNTupleWriter.hxx> | ||
| #include <TFile.h> |
There was a problem hiding this comment.
Put in include block after the <ROOT/...> includes
There was a problem hiding this comment.
Moved include block
| @@ -0,0 +1,70 @@ | |||
| #include <ROOT/RNTupleReader.hxx> | |||
| #include <iostream> | |||
There was a problem hiding this comment.
Move to include block after the ROOT includes
There was a problem hiding this comment.
Moved include block
| #include <ROOT/RNTupleReader.hxx> | ||
| #include <iostream> | ||
| #include <ROOT/RVersion.hxx> | ||
| using namespace std; |
There was a problem hiding this comment.
Please don't put using namespace std into new code
There was a problem hiding this comment.
Removed using namespace
| std::string_view output = "structure.attributes.json") { | ||
| #if ROOT_VERSION_CODE < ROOT_VERSION(6, 40, 0) | ||
| cout << "Skipped structure/attributes/read.C - ROOT version too old." << endl; | ||
| return void(); |
There was a problem hiding this comment.
just
| return void(); | |
| return; |
(or actually nothing at all, since the rest of the code is behind preprocessor guards)
There was a problem hiding this comment.
Removed return
| for (auto idx : reader->GetEntryRange()) { | ||
| for (auto attrIdx : attrSet->GetAttributes()) { | ||
| attrSet->LoadEntry(attrIdx, *attrEntry); |
There was a problem hiding this comment.
This prints all attributes for all entries in the main RNTuple. That's probably not what we want, also in light of Jakob's comment to have multiple attribute entries. We also need to print the attribute range(s)
| os << "}\n"; | ||
| return; | ||
| } catch (const runtime_error &e) { | ||
| cout << "Skipped structure/attributes/read.C - " << e.what() << endl; |
There was a problem hiding this comment.
Should we handle the case of missing structure.attributes.root more explicitly? That is the only error that I think is permissable, all others should make the test fail.
| @@ -0,0 +1,3 @@ | |||
| # Attributes | |||
|
|
|||
| A RNTuple with a regular field and a attribute field. | |||
There was a problem hiding this comment.
Please describe at least the attribute name and its field in more detail, similar to the explanations for other tests.
|
|
||
| void read(std::string_view input = "structure.attributes.root", | ||
| std::string_view output = "structure.attributes.json") { | ||
| #if ROOT_VERSION_CODE < ROOT_VERSION(6, 40, 0) |
There was a problem hiding this comment.
In general, please run clang-format on the C++ code.
undefined-panda
force-pushed
the
attributes-test
branch
from
f95bb9b to
f05022c
Compare
3 participants
Skip write.C for versions that don't support attributes (everything before 6.40) and read.C if binary for attributes doesn't exist.