[df] Make Report action mergeable and available in distributed RDF#21082
[df] Make Report action mergeable and available in distributed RDF#21082martamaja10 wants to merge 6 commits intoroot-project:masterfrom
Conversation
martamaja10
force-pushed
the
report-distr
branch
from
65d9af9 to
31457de
Compare
Test Results 22 files 22 suites 3d 9h 19m 0s ⏱️ Results for commit c992d09. ♻️ This comment has been updated with latest results. |
martamaja10
force-pushed
the
report-distr
branch
4 times, most recently
from
765b1d6 to
479af61
Compare
martamaja10
force-pushed
the
report-distr
branch
from
479af61 to
c4940d8
Compare
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
LGTM, mostly cosmetics below.
I have just one question:
What do we do, if by some accident or future extension the two cutflow reports don't contain the same cuts? Should we signal some kind of error or proceed with the match cuts by name and merge what can be merged, and leave untouched what cannot be merged?
| const auto pass = ci.GetPass(); | ||
| const auto all = ci.GetAll(); | ||
| const auto eff = ci.GetEff(); | ||
| const auto cumulativeEff = 100.f * float(pass) / float(allEntries); |
There was a problem hiding this comment.
Maybe this is better run in double. With float, one could arrive earlier at 0 or 100%, even if the true values were 99.9999999 or so.
There was a problem hiding this comment.
ok, I will change it, I'll change it here and in the Print function in a separate commit (maybe together with changing the space for the cut name)
| const auto eff = ci.GetEff(); | ||
| const auto cumulativeEff = 100.f * float(pass) / float(allEntries); | ||
|
|
||
| std::string stringtodisplay = Form("%-10s: pass=%-10lld all=%-10lld -- eff=%3.2f %% cumulative eff=%3.2f %%", |
There was a problem hiding this comment.
I wonder if one should allow a bit more space for the cut names? The number is of course arbitrary, but having a bit more space than for a number seems reasonable to me.
There was a problem hiding this comment.
I followed the choice that was there before (for Print() ), but it is true, we could leave more space, should I double it? or would idk 15 be enough?
martamaja10
force-pushed
the
report-distr
branch
from
c4940d8 to
43de9ca
Compare
Calculation of the efficiencies is now done on doubles instead of floats for increased precision. The space left for cut name has been increased.
martamaja10
force-pushed
the
report-distr
branch
from
43de9ca to
c992d09
Compare
vepadulano
left a comment
vepadulano
left a comment
There was a problem hiding this comment.
Thank you! I believe this is very close to what we want. I left a couple of minor comments. Could you also add a unittest to roottest/python/distrdf/backends/check_reducer_merge.py that checks the functionality also in distributed RDF?
| assert(strcmp(this->fCutInfoVec[i].GetName().c_str(), othercast.fCutInfoVec[i].GetName().c_str()) == 0 && | ||
| "Mergeable reports have different cut names."); |
There was a problem hiding this comment.
Isn't this assert redundant with the runtime_error being thrown a few lines below?
| namespace RDF { | ||
|
|
||
| void RCutFlowReport::Print() | ||
| void RCutFlowReport::Print() const |
There was a problem hiding this comment.
Ideally now that we have AsString this method should just forward to it.
3 participants
The report action has not been mergeable so far and hence it was not possible to use it in distributed RDF. This PR makes it possible. In order to make the testing easier, basic AsString method was added to the RCutFlowReport class.
Note: