diff --git a/bridge/sds/SdsClient.java b/bridge/sds/SdsClient.java index 7cc2a29..07e49a6 100644 --- a/bridge/sds/SdsClient.java +++ b/bridge/sds/SdsClient.java @@ -162,18 +162,43 @@ public final class SdsClient extends BotClient { int illegal; int failed; /** - * Individual refusals, which is not the same as refused decisions. + * Individual rejections, which is not the same as decisions that had + * one. * - *

{@code illegal} counts decisions that ended with the unit standing - * still; this counts the proposals MegaMek turned down on the way - * there, so it is at least as large. The gap between them is how much - * the re-ask is buying: equal means every refusal was fatal, and a - * larger {@code refusals} with a smaller {@code illegal} is a unit that - * was refused and then found something legal. + *

Counted per refusal on purpose. A decision may be refused + * up to {@link #MOVE_ATTEMPTS} times, and the whole reason the re-ask + * exists is that one rejected candidate can have many behind it. A + * count of decisions-that-were-refused would hide exactly the number + * worth having, so this can exceed {@code decisions} and is not part of + * the outcome partition below. + * + *

The gap to {@code neverAnswered} is what re-asking buys: equal + * means every refusal was fatal, and a larger {@code rejected} with a + * smaller {@code neverAnswered} is a unit that was refused and then + * found something legal. + */ + int rejected; + /** + * Decisions the bot declined to act on, having answered. + * + *

A reply arrived and it was not the action asked for - the bot + * passing, which is what `candidates::generate` does for a machine the + * rules will not move. **That is an answer**, and the commonest one: on + * one vehicle bench all 64 were an immobilised unit. Kept apart from + * {@link #unanswered} because "this machine does not move" and "nothing + * came back" are different facts about a match. + */ + int passed; + /** + * Decisions where nothing came back at all - a timeout, a dead bot, a + * seat that has been disabled. + * + *

{@code defaulted} is these two added together, and was the only + * number until it was asked to answer "what happened during this + * match". It cannot: a bot that passed correctly and a bot that never + * replied both land in it. */ - int refusals; - /** Decisions that used every attempt and were still refused. */ - int exhausted; + int unanswered; /** * Turn order. Summed over decisions, so the mean set size is * {@code eligible / decisions} - the number that says whether the bot @@ -430,7 +455,7 @@ public final class SdsClient extends BotClient { reply = ask(request); if (reply == null || !"move".equals(reply.path("kind").asText(""))) { // Not a refusal and not retried: see the note above. - tally.defaulted++; + countDeclined(tally, reply); record(request, reply, "defaulted", "stand still", mover.getId(), rejected); return new MovePath(getGame(), mover); } @@ -442,7 +467,7 @@ public final class SdsClient extends BotClient { record(request, reply, "answered", path.toString(), mover.getId(), rejected); return path; } - tally.refusals++; + tally.rejected++; ObjectNode refusal = rejected.addObject(); refusal.put("unit", mover.getId()); refusal.put("attempt", attempt); @@ -459,7 +484,6 @@ public final class SdsClient extends BotClient { // four different moves and had all four turned down is a different // report from one that was refused once. tally.illegal++; - tally.exhausted++; System.err.println("[sds] " + getName() + " unit " + mover.getId() + " had all " + MOVE_ATTEMPTS + " moves refused in round " + getGame().getCurrentRound() + "; standing still"); @@ -534,7 +558,7 @@ public final class SdsClient extends BotClient { try { reply = ask(request); if (reply == null || !"fire".equals(reply.path("kind").asText(""))) { - tally.defaulted++; + countDeclined(tally, reply); record(request, reply, "defaulted", "hold fire", shooter.getId()); } else { shooter = chosenActor(reply, eligible, first, tally); @@ -708,6 +732,25 @@ public final class SdsClient extends BotClient { * turn-order statistic that would then read past 100%. The first answer is * the bot's unforced pick; the retries are the host's doing. */ + /** + * Count a decision the bot did not answer with the action that was asked + * for, splitting the two things that look identical in a total. + * + *

{@code defaulted} is kept as the sum so every existing reader of it + * means what it always meant; {@code passed} and {@code unanswered} are the + * halves. A bot that declined because the rules will not move the machine + * has answered the question, and a bot that never replied has not - and + * only the second is a hole in a match's behaviour. + */ + private void countDeclined(Tally tally, @Nullable JsonNode reply) { + tally.defaulted++; + if (reply == null) { + tally.unanswered++; + } else { + tally.passed++; + } + } + private Entity chosenActor(JsonNode reply, List eligible, Entity fallback, @Nullable Tally tally) { if (!reply.has("unit")) { @@ -788,7 +831,7 @@ public final class SdsClient extends BotClient { + unit.getShortName() + " to " + asked + ", which is not a legal hex"); } } else { - tally.defaulted++; + countDeclined(tally, reply); outcome = "defaulted"; } } catch (Exception e) { @@ -862,7 +905,7 @@ public final class SdsClient extends BotClient { try { reply = ask(request); if (reply == null || !"fire".equals(reply.path("kind").asText(""))) { - tally.defaulted++; + countDeclined(tally, reply); record(request, reply, "defaulted", "no physical", first.getId()); return null; } @@ -1550,8 +1593,9 @@ public final class SdsClient extends BotClient { int pinnedNoMp = 0; int pinnedLimb = 0; int timedOut = 0; - int refusals = 0; - int exhausted = 0; + int rejected = 0; + int passed = 0; + int unanswered = 0; ObjectNode phases = mapper.createObjectNode(); for (Map.Entry entry : tallies.entrySet()) { Tally t = entry.getValue(); @@ -1570,18 +1614,23 @@ public final class SdsClient extends BotClient { pinnedNoMp += t.pinnedNoMp; pinnedLimb += t.pinnedLimb; timedOut += t.timedOut; - refusals += t.refusals; - exhausted += t.exhausted; + rejected += t.rejected; + passed += t.passed; + unanswered += t.unanswered; ObjectNode phase = phases.putObject(entry.getKey()); phase.put("decisions", t.decisions); phase.put("answered", t.answered); phase.put("defaulted", t.defaulted); phase.put("illegal", t.illegal); - // Refusals rather than refused decisions, and decisions that used - // every attempt. `illegal` counts decisions that ended standing - // still; the gap to `refusals` is what re-asking bought. - phase.put("refusals", t.refusals); - phase.put("exhausted", t.exhausted); + // Individual rejections, which can exceed `decisions` and is not + // part of the partition below. + phase.put("rejected", t.rejected); + // The two halves of `defaulted`, and the roll-up that says how much + // of a match went unanswered. `neverAnswered` is derived rather + // than counted so it cannot drift from its parts. + phase.put("passed", t.passed); + phase.put("unanswered", t.unanswered); + phase.put("neverAnswered", t.unanswered + t.illegal + t.failed); phase.put("failed", t.failed); phase.put("eligible", t.eligible); phase.put("chose", t.chose); @@ -1598,8 +1647,10 @@ public final class SdsClient extends BotClient { n.put("answered", answered); n.put("defaulted", defaulted); n.put("illegal", illegal); - n.put("refusals", refusals); - n.put("exhausted", exhausted); + n.put("rejected", rejected); + n.put("passed", passed); + n.put("unanswered", unanswered); + n.put("neverAnswered", unanswered + illegal + failed); n.put("failed", failed); // Turn order, in total. `eligible` is a sum, not a count of decisions: // divided by `decisions` it is the mean size of the set the bot was diff --git a/plan/protocol.md b/plan/protocol.md index 89332d8..5c2208a 100644 --- a/plan/protocol.md +++ b/plan/protocol.md @@ -297,12 +297,43 @@ MegaMek's own diagnosis, and each is recorded individually in the decision's that says which rule was missed - and this repository has just spent a night on records that summarised away the thing somebody needed. -The tally splits what used to be one number: `refusals` counts proposals turned -down, `illegal` counts decisions that ended standing still, and `exhausted` -counts those that used every attempt. `sds stats` prints the gap as `retries` - -refusals that were *not* fatal. **A zero there beside a non-zero `refused` means -re-asking is buying nothing**, which would be a finding about candidate -generation rather than about this loop. +### What a match's behaviour is made of + +jmm asked for a `rejected` count and a `never answered` count beside +`answered`/`decisions`, "to get a full idea of what happened with behavior +during a match". Two of the three were already here under other names; the third +was hidden inside a number that could not tell two things apart. + +- **`rejected` counts individual rejections, not decisions that had one.** A + decision may be refused four times, and the reason the loop exists is that one + rejected candidate can have many behind it - so this can exceed `decisions` + and is deliberately outside the outcome partition. It was `refusals`; the name + is jmm's word and the semantics were already right. +- **`neverAnswered` is a decision that ended with no accepted move**, derived as + `unanswered + illegal + failed`. Derived rather than counted so it cannot + drift from its parts. +- **`passed` is not one of them.** A bot that declines because the rules will not + move the machine *has answered*: `candidates::generate` offers nothing for an + immobilised unit, and on one vehicle bench all 64 such decisions were exactly + that. Counting it as never-answered would report the design working as a + failure. + +`defaulted` could not make that distinction, and that is what this change is +really about: **a reply that never came and a bot that deliberately passed both +landed in it.** It is now the sum of `passed` and `unanswered`, kept so every +existing reader means what it always meant, with the halves beside it. + +**`exhausted` is gone.** It was incremented on the same line as `illegal` and +was always equal to it within a phase - a duplicate counter, written by the same +change that added the one it duplicates, which is the defect the review had just +caught in a different form. + +`answered + passed + neverAnswered == decisions`, and +`test_the_outcomes_account_for_every_decision` reads the client's source and +fails on any `tally.++` the partition does not know about. **That is the +guard that matters**: an outcome added later without a counter would otherwise +vanish into the gap rather than announce itself, which is precisely how two +counters here once read zero forever and were believed. **Two counting faults the loop introduced and the review caught.** Neither would have thrown; both would have produced a plausible number. @@ -314,7 +345,7 @@ have thrown; both would have produced a plausible number. retries are the host's doing, not the bot's pick. - The counter below. -**One counter nearly shipped dead.** `refusals` and `exhausted` were added to +**One counter nearly shipped dead.** `rejected` and its neighbour were added to the tally and to `runstats.py`'s `COUNTS` and left out of the JSON the seat writes. Nothing failed: `runstats` reads counters out of a `Counter`, so a name the client never writes reads **0** for every match forever - the column would @@ -326,4 +357,5 @@ the same reason. - [ ] Measure how many attempts a real match actually needs. If it is usually one, four is generous; if units routinely burn all four, the interesting question is why the menu's second and third entries are refused too. - `retries` in `sds stats` is the figure: refusals that were not fatal. + `recovered` in `sds stats` is the figure: rejections that were not + fatal, `rejected - neverAnswered`. diff --git a/sds/runstats.py b/sds/runstats.py index 7c87e72..d8b56b6 100644 --- a/sds/runstats.py +++ b/sds/runstats.py @@ -10,7 +10,8 @@ Two sources, and the difference matters when a number looks wrong: - **The match results.** `player["sds"]["phases"][phase]` is written by the bot through the harness and is as reliable as the run itself. Decisions, what was - answered, what was defaulted, what the host refused. + answered, what the bot declined, what the host rejected, and what was never + answered at all. - **The match logs.** The per-unit `[sds-bot]` lines on stderr. A secondary source: a killed match, a rotated log or a changed format loses lines silently, so the rollup reports how many matches it could read rather than @@ -38,12 +39,15 @@ COUNTS = ( "defaulted", "illegal", "failed", - # Refusals rather than refused decisions, and decisions that used every - # attempt. `illegal` counts decisions that ended standing still; `refusals` - # counts the proposals turned down on the way there, so the gap between - # them is what re-asking bought. See `SdsClient.askMovement`. - "refusals", - "exhausted", + # Individual rejections, which can exceed `decisions`: a decision may be + # refused up to four times, and one rejected candidate can have many behind + # it. Not part of the outcome partition. + "rejected", + # The two halves of `defaulted` - the bot declining, and nothing coming + # back - and the roll-up of everything that ended with no accepted move. + "passed", + "unanswered", + "neverAnswered", "eligible", "chose", "differed", @@ -323,25 +327,25 @@ def render(out: Rollup) -> str: lines.append(f"[{kind}] {seat.matches} seats played") header = ( f" {'phase':<11} {'decisions':>9} {'answered':>9} {'defaulted':>9} " - f"{'refused':>8} {'retries':>8}" + f"{'rejected':>9} {'never':>7} {'recovered':>10}" ) lines.append(header) for phase in PHASES: if phase not in seat.phases: continue counts = seat.phases[phase] - refused = counts["illegal"] + counts["failed"] - # Refusals that were not fatal: the decision was refused and asked - # again, and something legal came back. A zero here with a non-zero - # `refused` means re-asking is buying nothing, which is a finding - # about candidate generation rather than about the loop. - recovered = max(0, counts["refusals"] - counts["exhausted"]) + # Rejections that were not fatal: the decision was refused, asked + # again, and something legal came back. A zero here beside a + # non-zero `rejected` means re-asking is buying nothing, which is a + # finding about candidate generation rather than about the loop. + recovered = max(0, counts["rejected"] - counts["neverAnswered"]) lines.append( f" {phase:<11} {counts['decisions']:>9} " f"{_rate(counts['answered'], counts['decisions']):>9} " f"{_rate(counts['defaulted'], counts['decisions']):>9} " - f"{_rate(refused, counts['decisions']):>8} " - f"{recovered:>8}" + f"{counts['rejected']:>9} " + f"{_rate(counts['neverAnswered'], counts['decisions']):>7} " + f"{recovered:>10}" ) move = seat.phases.get("movement", Counter()) if move["eligible"]: diff --git a/tests/test_decision_record.py b/tests/test_decision_record.py index f463c95..9698c87 100644 --- a/tests/test_decision_record.py +++ b/tests/test_decision_record.py @@ -80,13 +80,96 @@ class TestSeatCounters(unittest.TestCase): "writes into a phase - it would read 0 rather than fail", ) + def test_the_outcomes_account_for_every_decision(self) -> None: + """Every decision lands in exactly one outcome, and the sum says so. + + `answered`, `passed` and `neverAnswered` partition `decisions`, where + `neverAnswered` is `unanswered + illegal + failed`. The point is not the + arithmetic - it is that an outcome added later without a counter fails + here instead of vanishing into the gap, which is exactly how `rejected` + and `exhausted` once read zero forever and were believed. + + Checked against the source rather than a run: every `tally.++` inside + a decision method has to be one of the names the partition knows. + """ + text = CLIENT.read_text() + known = { + "decisions", + # the partition + "answered", + "passed", + "unanswered", + "illegal", + "failed", + "defaulted", + # counted per rejection, deliberately outside it + "rejected", + # bookkeeping, not outcomes + "eligible", + "chose", + "differed", + "substituted", + "prone", + "pronePinned", + "pinnedGyro", + "pinnedNoMp", + "pinnedLimb", + "timedOut", + } + seen = set(re.findall(r"tally\.(\w+)\+\+", text)) + self.assertEqual( + seen - known, + set(), + "a counter is incremented that the outcome partition does not know " + "about; add it to the partition or to the bookkeeping list, or it " + "will not be accounted for in any match", + ) + + def test_never_answered_is_derived_from_its_parts(self) -> None: + """`neverAnswered` is computed, not counted, so it cannot drift. + + A separately incremented counter is how two numbers that should agree + stop agreeing. This one is written as the sum of the three things it + rolls up, in both the per-phase object and the seat total. + """ + text = CLIENT.read_text() + self.assertIn( + "t.unanswered + t.illegal + t.failed", + text, + "the per-phase neverAnswered is no longer derived from its parts", + ) + self.assertIn( + "unanswered + illegal + failed", + text, + "the seat-total neverAnswered is no longer derived from its parts", + ) + + def test_declining_and_not_answering_are_counted_apart(self) -> None: + """A bot that passed and a bot that never replied are different facts. + + `defaulted` conflated them, and on one vehicle bench all 64 of its + movement decisions were an immobilised unit answering correctly. An + immobilised unit must not read as never-answered. + """ + text = CLIENT.read_text() + self.assertIn("tally.unanswered++", text) + self.assertIn("tally.passed++", text) + # And nothing increments `defaulted` except the helper that splits it. + bare = [n for n, line in enumerate(text.split("\n"), 1) if "tally.defaulted++" in line] + self.assertEqual( + len(bare), 1, f"defaulted is incremented outside countDeclined: lines {bare}" + ) + def test_the_retry_counters_reach_the_json(self) -> None: """Named, because they are the ones that were missing. - `refusals` and `exhausted` are what say whether re-asking a refused move - buys anything. Silently zero, they would have said it never happens. + `rejected`, `passed`, `unanswered` and `neverAnswered` are what say what + happened during a match. Silently zero, they would have said none of it + ever happens. """ - self.assertLessEqual({"refusals", "exhausted"}, phase_counters()) + self.assertLessEqual( + {"rejected", "passed", "unanswered", "neverAnswered"}, phase_counters() + ) class TestDecisionRecord(unittest.TestCase):