Skip to content

Docs: reword the froude README's claim about the two constructions - #155

Open
kawh1111 wants to merge 1 commit into
Flood-Lab:mainfrom
kawh1111:docs/froude-scored-wording
Open

kawh1111 wants to merge 1 commit into
Flood-Lab:mainfrom
kawh1111:docs/froude-scored-wording

Conversation

@kawh1111

@kawh1111 kawh1111 commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator

Docs only, one sentence.

probes/momentum/froude-regime/README.md said the two constructions "fail again, on all 1460 steps, with every one of those steps scored on the runoff", which reads as the 1,460 steps being the ones that fail. What was measured — and what the paragraph around it already says — is that the record fails on all three gate seeds and that all 1460 steps are scored, with 1,316 of them exceeding on the first gate seed.

Raised by @cehw in his approving review of #103 (review 5348903391), where it was acknowledged as a wording error rather than a measurement one and left to "the next commit that touches this file" rather than a push that would have dismissed the approval it was correcting. #103 merged before such a commit existed, so here it is.

ht validate passes (34 probes, 77 models), and pytest tests/test_docs_in_sync.py tests/test_froude.py tests/test_report_detail.py -q is 317 passed. No criterion, probe, test or archive row changes, so no verdict moves and no archived detail changes.

The sentence read "Both constructions fail again, on all 1460 steps, with every
one of those steps scored on the runoff", which says the 1,460 steps are the
ones that fail. What was measured is that the record fails on all three gate
seeds and that all 1460 steps are scored on the runoff — 1,316 of them
exceeding on the first gate seed. Raised by @cehw in his approving review of
Flood-Lab#103 (review 5348903391) and acknowledged there as a wording error rather than
a measurement one, to be fixed in the next commit that touched this file.

Docs only: one sentence, no criterion, probe or test changes, so no verdict and
no archived row moves.
@kawh1111
kawh1111 requested a review from chrimerss as a code owner October 1, 2026 05:30
@chrimerss

Copy link
Copy Markdown
Contributor

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by Claude (no-blocking-issues), not a maintainer approval.

One sentence of probes/momentum/froude-regime/README.md is reworded. The old text said the two dis-shrinking constructions "fail again, on all 1460 steps, with every one of those steps scored on the runoff", which reads as all 1,460 steps being failures; the new text says all 1460 steps are scored on the runoff, with 1,316 of them exceeding on the first gate seed.

The new number checks out against two independent places in the tree, which is the main thing worth knowing:

  • README.md:217 records that zeroing dis on exactly the supercritical steps left 144 of 1460 scored on the first gate seed (the same figure is in src/hydroturing/criteria/froude.py:392 and the test_froude.py:270 docstring), so the must-fail is supercritical on 1460 − 144 = 1316 steps there.
  • 1316/1460 = 90.1%, exactly the top of the "83.3–90.1% of scored steps" range at README.md:110. The other two seeds, 1460 − 154 and 1460 − 244, give 89.5% and 83.3%, closing the range.

Attributing one count to both constructions is right: both scale dis only, so the larger reading is mrro on every step (steps_read_from_second_reading == 400 in test_froude.py:323), and mrro matches dis to 8.2e-13 relative, so the record "lands exactly on the unmodified must-fail".

No criterion, probe, test or archive row moves, so nothing on the AGENTS.md merge checklist applies and no docs test covers this prose. Nothing in the PR text asks for a particular outcome.

Non-blocking: the sentence writes 1,316 next to 1460; this file groups nothing (1460, 1400, 3137, 438).

Posted by the pr-review workflow. Run

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants