[RDF] Fix the events/s figure in the progress bar's final line - #23065
Conversation
PrintStatsFinal divided the event count by an elapsed time that had already been truncated to whole seconds, so an event loop shorter than one second divided by zero and printed `inf evt/s`, and a longer one reported a rate quantised upwards by the truncation. Keep the truncated value for the h:mm display and divide by the full-precision duration instead. The in-run line is unaffected: it already computes its rate from a duration<double> in RecordEvtCountAndTime. Adds a regression test that fails on the current code with `inf evt/s` and passes with this change.
|
Six days in, this PR has had no CI at all — not a red run, no run. That is the outside-contributor Four workflow runs exist against
Restating the change so nobody has to re-read the diff — two files, +27/-3:
Still applies cleanly to @vepadulano (assigned) / @hageboeck (last touched the progress bar) — whenever one of you has a |
Test Results 23 files 23 suites 3d 15h 12m 33s ⏱️ For more details on these failures, see this check. Results for commit 4a11a85. |
|
Hi @kutsibalci , Thanks for your contribution and apologies for the longer wait caused by the assignees being on leave. I see the CI was approved and ran, reporting only a few unrelated test failures, so your changes don't seem to break any existing test. I will review your code and comment accordingly. |
vepadulano
left a comment
There was a problem hiding this comment.
Changes LGTM, but see a minor request for the test.
Requested in review. The progress bar rate regression test wrote fh_rate.root into the working directory and left it behind. Wrapping the path in ROOT::TestSupport::FileRaii removes it when the scope exits, whether the test passes or throws. ROOT/TestSupport.hxx was already included by this file, so this adds no new dependency. Rebuilt and ran the suite: 33/33 pass and fh_rate.root is gone afterwards.
vepadulano
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
This Pull request:
Changes or fixes:
ProgressHelper::PrintStatsFinalcomputes the events/s figure from a duration that has already been truncated to whole seconds:duration_casttruncates toward zero, soelapsedSeconds.count()is an integer. Two consequences:inf evt/s;Measured on this branch's parent, one 400-entry tree with a
Filterthat spins for a fixed time per entry, so the real rate is known:masterprintsinf evt/s8.94e+02 evt/s2.00e+02 evt/s(+20%)1.66e+02 evt/s8.00e+01 evt/s(+12%)7.13e+01 evt/sThe fix keeps the truncated value for the
h:mmdisplay, which wants whole seconds, and divides by the full-precision duration:prettyPrint(elapsedSeconds)is unchanged, so the elapsed-time field prints exactly as before.The in-run line is not affected and is not touched.
PrintProgressAndStatstakes its rate fromEvtPerSec(), which averages the per-interval rates recorded inRecordEvtCountAndTimeusing aduration<double>. Only the final line had the integer division.This is independent of #15323. That issue is about the numerator on the same line —
ComputeTotalEvents()reports the size of the input dataset rather than what was processed. This PR deliberately does not touch the numerator; whichever way #15323 is decided, the denominator has to be a real duration.Reproducer used for the table
The counts go to a file because the progress bar clears the terminal line with
\rplus padding and swallows anything printed beside it.Checklist:
Built from source at
6b57f8fc(ninja, gcc 13,-O3) and exercised throughtree/dataframe/test/dataframe_helpers:RDFHelpers.ProgressBarFinalRateIsFinitefails:33 tests from 2 test suites ran. [ PASSED ] 33 tests.— includingProgressBarRestorePrecision, which is the other test that inspects this stream.This PR fixes # — no issue filed; happy to open one if you would rather track it that way.
AI disclosure
AI-assisted (Claude Code). The tool found the truncation while I was reading
PrintStatsFinalfor #15323, wrote the reproducer and the regression test, and drafted this description. I built ROOT from source, ran the reproducer and the test suite myself, and checked the before/after numbers in the table against the measured wall-clock rates. I have reviewed and understood the change and take responsibility for it.