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.
| 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.
There was a problem hiding this comment.
AFAICT the code hasn't changed, a missing structure.attributes.root is not handled explicitly...
There was a problem hiding this comment.
Explicit error handling for missing file was added.
undefined-panda
force-pushed
the
attributes-test
branch
3 times, most recently
from
0fa2cd4 to
047cee5
Compare
| } else { | ||
| os << " }\n"; | ||
| } | ||
| std::cout << " attr = " << *attr << " (valid for range [" << *range.GetFirst() << ", " << *range.GetLast() << "])\n"; |
| // same logic as is read_structure.hxx, with additional special part for | ||
| // attributes |
There was a problem hiding this comment.
Actually, the latest code doesn't print the main entry anymore. Probably we don't need this, but then we should update / remove this comment.
There was a problem hiding this comment.
Comment updated
| auto Int32 = | ||
| reader->GetModel().GetDefaultEntry().GetPtr<std::int32_t>("Int32"); |
| // list of values for attr with its range | ||
| os << " {\n"; | ||
| os << " \"Attr\": " << "[\n"; | ||
| first = true; |
There was a problem hiding this comment.
still already set above - please remove
|
|
||
| ## Fields | ||
|
|
||
| * `Int{32}` |
There was a problem hiding this comment.
| * `Int{32}` | |
| * `Int32` |
| os << "}\n"; | ||
| return; | ||
| } catch (const runtime_error &e) { | ||
| cout << "Skipped structure/attributes/read.C - " << e.what() << endl; |
There was a problem hiding this comment.
AFAICT the code hasn't changed, a missing structure.attributes.root is not handled explicitly...
| 1. Attribute set name | ||
| 2. Attribute set description | ||
| 3. List of attribute entries with value and regular field range |
There was a problem hiding this comment.
These are not (RNTuple) entries. What you want to describe here is that there is one attribute entry spanning one main entry, and the second attribute entry spanning two main entries.
There was a problem hiding this comment.
The first two items of the list are still there. This section of the README should describe what is written / read from the RNTuple data, ie "unroll" the third item.
| ## Fields | ||
|
|
||
| * `Int{32}` | ||
| * `Attr` |
There was a problem hiding this comment.
In the code it's "attr" (lower-case). Maybe you can also make it explicit that Int32 is the field in the main RNTuple, and attr is the field in the attribute model
undefined-panda
force-pushed
the
attributes-test
branch
from
e84cbd2 to
93792a4
Compare
| // list of values for attr with its range | ||
| os << " {\n"; | ||
| os << " \"Attr\": " << "[\n"; | ||
| first = true; |
There was a problem hiding this comment.
still already set above - please remove
| 1. Attribute set name | ||
| 2. Attribute set description | ||
| 3. List of attribute entries with value and regular field range |
There was a problem hiding this comment.
The first two items of the list are still there. This section of the README should describe what is written / read from the RNTuple data, ie "unroll" the third item.
|
|
||
| // definition of attribute set with attribute fields | ||
| auto attrModel = ROOT::RNTupleModel::Create(); | ||
| attrModel->SetDescription("Attribute set for testing"); |
There was a problem hiding this comment.
Actually do we want to test this? Right now we don't do it for the main RNTuple descriptor, nor for the field descriptions etc...
| auto attr = attrSet->GetModel().GetDefaultEntry().GetPtr<std::int32_t>("attr"); | ||
|
|
||
| os << " {\n"; | ||
| os << " \"Attribute set name\": \"" << attrSet->GetDescriptor().GetName() << "\"\n"; |
There was a problem hiding this comment.
This is not really useful since the name is already hard-coded above
| os << " \"Attribute set name\": \"" << attrSet->GetDescriptor().GetName() << "\"\n"; | ||
| os << " }, \n"; | ||
| os << " {\n"; | ||
| os << " \"Description\": \"" << attrSet->GetDescriptor().GetDescription() << "\"\n"; |
| os << " {\n"; | ||
| os << " \"Attr\": " << "[\n"; | ||
| first = true; | ||
| for (auto attrIdx : attrSet->GetAttributes()) { |
There was a problem hiding this comment.
These entries should be the main content of the .json file
| * `Int32` (in main RNTuple) | ||
| * `attr` (in attribute model) | ||
|
|
||
| one regular and one attribute field. |
There was a problem hiding this comment.
This information is now in the list. However, we should describe the field types (std::int32_t)
| try { | ||
| reader = ROOT::RNTupleReader::Open("ntpl", input); | ||
| } catch (const ROOT::RException &e) { | ||
| std::ofstream os(std::string{output}); | ||
| std::string msgWithoutStacktrace; | ||
| std::getline(std::istringstream(e.what()), msgWithoutStacktrace); | ||
| os << "{\n"; | ||
| os << " \"error\": \"" << msgWithoutStacktrace << "\"\n"; | ||
| os << "}\n"; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Actually I think we can simplify this: in read_structure.hxx, we catch exceptions because structure/feature_flag/read.C requires it. Here we can go back to the naive code of just calling RNTupleReader::Open and in case of exceptions let the entire macro fail.
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.