fix(partition): 🔇 collapse identical COMMPLACE repeats - #286
Merged
Conversation
Making the line unconditional exposed its volume: the Python suite emitted 437 COMMPLACE lines in one 585-test serial run, and all 437 were the same string. One process has one placement, so the repeats are noise, not data. `place_line_is_new` compares the whole LINE rather than setting a once-flag: a process that builds an `alone` propagator before an MPI one has two placements and both are worth saying. Non-template so the guard is process-wide rather than one per NumModes instantiation, and returned rather than written so the collapse is testable without capturing stderr. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
diagonal-hamiltonian
requested review from
fpietra,
ludmilaasb and
robertodr
as code owners
August 25, 2026 14:48
|
Docs preview: https://pr-286.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 14 14
Lines 742 742
Branches 98 98
=======================================
Hits 725 725
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
SonarQube cpp:S6009 on the new signature. The function only compares and assigns, so it never needed to own or bind a std::string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Panadestein
approved these changes
Aug 26, 2026
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



🤖 AI text below 🤖
Summary
Follow-up to #269, which made the
COMMPLACEline unconditional at review request. That change iscorrect and I am not walking it back — but making it unconditional exposed its volume, and the
volume is a defect:
One serial 585-test Python run emits 437
COMMPLACElines, and all 437 are the same string.A process has one placement. Reporting it 437 times is noise, not data, and it is on
mainnow.fix/commplace-collapse-repeatsf87b662, 2 commits on34db77d, 4 files +42/−8.Why #269 did not catch this
Because it measured with
pytestcapturing. #269's body states that "the whole Python suite emitsnothing" — that reading came from a run without
-s, and pytest's fd capture swallows C++stderr.Same arm, same command,
-sthe only difference:COMMPLACElinespytest … -qpytest … -q -sThe zero was capture, not silence. Any future measurement of this line needs
-s, and the probescript now says so in a comment rather than leaving the next person to rediscover it.
The fix
place_line_is_new(line)returns true the first time a line is seen and whenever it differs from theprevious call;
PartitionGroupwrites only when it does.Three choices in it worth stating, because the obvious implementations are each wrong in a way:
std::call_oncewould report the firstplacement only. A process that builds an
alonepropagator before an MPI one has two placementsand both are worth saying; a changed line still gets through.
CpuTopology.cpp, not a static local inPartitionGroup<NumModes>. A static inside the template gets one guard perNumModesinstantiation, so the collapse would leak one line per distinct mode count.
testable without capturing
stderr— which is exactly the instrument that hid the problem.The second commit takes the argument by
std::string_view(SonarQubecpp:S6009on the first): thefunction only compares and assigns, so it never needed to bind a
std::string.No knob, no behaviour change to what a distinct placement reports, and nothing branches on it.
Measurement
Arm md5
f34e996d. The counts are the point of the PR, so they are the measurement:34db77d)d7ff600)-sGates on
f34e996d:ctest -L unit239/239,-L serial238/238, jobCOMPLETED 0:0. The unitcount is one above
main's 238 because of the new case. Python suite 585 passed, 8 deselected.The
string_viewcommit is a signature change only; its case still asserts 6 of 6 and the countsabove re-measured identically.
CI: all 16 checks green, clang-tidy 0 unsuppressed warnings, SonarQube 0 open issues.
The new case asserts 6 of 6 (
--report_level=detailed) and covers the two failure modes separately:a repeat is suppressed, and a changed line is not — the second is what a
call_onceimplementationwould fail.
Ground-truth multi-rank behaviour is unchanged and re-verified with no env var set; the collapse is
per process, so co-located ranks still each report:
These runs are not reproducible from the diff: they need the Deucalion harness, which is not in this
repo.
Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicableDocs unchanged on purpose:
parallelism.mdxalready says the line is written per rank at propagatorconstruction with no knob, and that stays true.
AI/LLM disclosure
Important
By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.
Warning
If you're contributing on behalf of your employer, contact cla@algorithmiq.fi to arrange a Corporate CLA.