add node summary string methods #698
Conversation
| std::string pad = " "; | ||
| std::string eoe = "\n"; | ||
|
|
||
| if(opts.has_child("num_children_threshold") && |
There was a problem hiding this comment.
I know we're using num_children and num_elements because they're standard Conduit vernacular, but could we use something a bit shorter than threshold since it isn't as standard? Maybe something short, like max?
There was a problem hiding this comment.
I used threshold b/c that is what I settled on for the DataArray class (mirrors numpy's param name)
There was a problem hiding this comment.
|
|
||
| //----------------------------------------------------------------------------- | ||
| std::string | ||
| Node::to_summary_string(const conduit::Node &opts)const |
There was a problem hiding this comment.
Is the idea that the other conduit::Node::to_* methods are moving to an options conduit::Node approach? This seems like a break from convention otherwise.
There was a problem hiding this comment.
I would like to move in that direction, number of options is getting large.
| // NOTE(cyrush): In this case to_summary_string() is easily callable | ||
| // in debugging tools, but still provide the to_summary_string_default() | ||
| // variant to present a similar interface to other `to_` methods. | ||
| std::string to_summary_string_default() const; |
There was a problem hiding this comment.
As an FYI, I found a way to use even the default-parameter conduit::Node::to_* functions in gdb, so I think we can delete these conduit::Node::to_*_default functions in 0.7.X.
There was a problem hiding this comment.
ok, lets discuss further the path post 0.7.0 release. I'll keep this here for now.
| num_children_threshold = (index_t)opts["num_children_threshold"].to_int32(); | ||
| } | ||
|
|
||
| if(opts.has_child("num_elements_threshold") && |
There was a problem hiding this comment.
Might it be useful to include a max/threshold on tree depth as well? I could see it being useful to get a broad idea of the basic building blocks of a schema. Not saying we need to do it for this PR, but something to think about.
There was a problem hiding this comment.
i agree, we should do that in the future.
| bb: [1, 1, ..., 8, 9] | ||
| ... ( skipped 1 child ) | ||
| dd: [3, 1, ..., 8, 9] | ||
| )ST"; |
There was a problem hiding this comment.
If regex doesn't work out, I think I'm going to use this strategy on my output checks as well.
| opts.print(); | ||
| tres = n.to_summary_string(opts); | ||
| std::cout << tres << std::endl; | ||
| EXPECT_EQ(tres,n.to_yaml()); |
There was a problem hiding this comment.
I hadn't considered this case; good coverage of the possible inputs!
| bool done = (nchildren == 0); | ||
| int idx = 0; | ||
|
|
||
| while(!done) |
There was a problem hiding this comment.
The code in these loops is rather hard for me to follow. I think using the lambdas we're afforded by C++11 could make it much more legible, e.g.:
const auto to_summary = [] (const conduit::Node& n, ...)
{
// ... //
}
for(index_t idx = 0; idx <= bottom; idx++)
{
to_summary(node);
}
// ... skipped print logic goes here ... //
for(index_t idx = nchildren - top; idx < nchildren; idx++)
{
to_summary(node);
}
Just my 2 cents here; I don't mind keeping this as is for the sake of expediency.
There was a problem hiding this comment.
(i think this is great idea)
resolves #674
todo: