Adding the option to print values in the print action #63
Conversation
stefaniereuter
requested review from
geier1993,
tweska,
dsarmany and
MircoValentiniECMWF
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #63 +/- ##
===========================================
- Coverage 53.06% 53.04% -0.03%
===========================================
Files 217 217
Lines 14644 14656 +12
Branches 1213 1216 +3
===========================================
+ Hits 7771 7774 +3
- Misses 6873 6882 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -21,6 +21,8 @@ namespace multio::action { | |||
| Print::Print(const ComponentConfiguration& compConf) : ChainedAction(compConf) { | |||
| stream_ = compConf.parsedConfig().getString("stream", "info"); | |||
| onlyFields_ = compConf.parsedConfig().getBool("only-fields", false); | |||
| includeValues_ = compConf.parsedConfig().getBool("include-data-values",false); | |||
| numberOfValues_ = compConf.parsedConfig().getInt("value-count",0); | |||
There was a problem hiding this comment.
Can we add a start-index option (default to 0) to indicate at which index to start printing? I think this would be especially useful if the data starts with missing values.
There was a problem hiding this comment.
do we know from which index missing values begin/end?
There was a problem hiding this comment.
I guess you can look at the bitmap and count. We could automate this and accept this option to be set to "first-non-missing", but that requires a bit more work.
There was a problem hiding this comment.
I would probably rather just have a mechanism to print missing values, i.e. write the string 'missing' or 'n/a' instead of a numeric value.
| (*os_) << msg << "\n"; | ||
| if(includeValues_) { | ||
| const auto* data = static_cast<const double*>(msg.payload().data()); | ||
| const auto* end = data + (msg.payload().size() / sizeof(double)); |
There was a problem hiding this comment.
I think our data can also be in 32 bits float instead of 64 bits double. In that case we should check the precision key and handle floats slightly different from doubles.
There was a problem hiding this comment.
ah you're right. That was a hack that I did because I knew it was a double field that came in. We should discuss if we want to have this in at all.
There was a problem hiding this comment.
Yeah, I think this would only be useful for tiny payloads. I guess there may not be harm including it, but the code will need a bit of tightening up beforehand. Apart from the precision support, we may want to make sure that we always use reference to ostream and a bit more 'modern' C++ for loops.
|
I have used it and before deleting it after this debug work I thought I'll bring it up for discussion if we want to have it added. But I agree that the precision part would need some work. I've also already added an filter that only fields are allowed because currently flushes would try to print a payload as well |
This is something I sometimes used when debugging actions. I also have a different version in another branch, where I added an option to output just flushes. I will add this in another PR as it's orthogonal. Need to write a test to move from draft to mergable, unless you all decide you don't want this option in the print action
How it works:
prefix: "Prefix"
stream: cout
include-data-values: true
value-count: 10