Skip to content

feat: param(key, name[, T]) — closes #32 - #33

Merged
sotashimozono merged 4 commits into
mainfrom
feat/typed-param-access
Sep 8, 2026
Merged

sotashimozono merged 4 commits into
mainfrom
feat/typed-param-access

Conversation

@sotashimozono

@sotashimozono sotashimozono commented Sep 8, 2026 •

Copy link
Copy Markdown
Member

Closes #32.

work_fn opened with Float64(key.params["system.kbT"]), and both halves of that were a hazard.

param(key, "system.kbT")            # dotted
param(key, "kbT")                   # the leaf, when only one group has it
param(key, "a")                     # AmbiguousPathKeyError, naming ["model", "system"]
param(key, "system.kbt")            # error listing what the key does carry
param(key, "system.a", Float64)     # 2.0
param(key, "system.kbT", Int)       # InexactError, at the boundary

A misspelled name was a bare KeyError naming only the string that was wrong — raised inside the
work function, on a worker, dispatched by run!. The config with the right spelling and the
function with the wrong one are in different files, and nothing compares them until a point is
computed.

The review changed more than it confirmed

Six passes over this branch. The substantive findings, all reproduced before acting on them:

A bare catch was swallowing everything. _resolve_axis_value caught the resolver's two
intended errors and also InterruptException, and any future bug. The consequence is concrete:
build_axis_formats returns an empty Dict, format_path falls back to fixed2, and 0.006 and
0.008 both render as sysg0.01 — one run's directory overwriting another's, which is the failure
format.jl's own header says auto mode exists to prevent.

Fixed by removing the try rather than narrowing it. _lookup_name returns the outcome — key,
groups, or nothing — and _resolve_name decides what to raise. The caller for which absent and
ambiguous are ordinary now asks instead of catching.

param(key, name, T) did not keep its own promise. It said it refuses a conversion that changes
the number; convert only does that for integer targets. Measured:

convert(Float64, 9007199254740993)  ->  9.007199254740992e15   silently one less
convert(Float32, 2.1)               ->  2.1f0                  silently rounded

Now converted and checked, so the promise holds for every T. The test fixture had to be replaced
to show it: 2.5 is exact in Float32, so the first version asserted a refusal on a value that
cannot lose anything and would have passed for the wrong reason.

The version was wrong. 0.5.0 for a purely additive change, while DataVault pins
ParamIO = "0.3, 0.4" — the next layer of the stack could not have resolved it at all. 0.4.8.

Three comments claimed things that were not true. The header said the rule's copies "drifted:
two raise AmbiguousPathKeyError and two raise a bare error". It is one typed, two bare, and one
that raised nothing at all — and erasing that last one hid the larger difference. The catch
comment said this was "the one caller" that swallows; _safe_format_path in diagnose.jl does the
same. A testset title claimed parity with the path builder, which raises a different exception for
the same condition.

Two functions the rewrite touched had no test that reached them. Every existing call to
build_axis_formats passes fully dotted path_keys, which take the exact-match short-circuit — so
the leaf branch and both failure paths were unexercised, and "behaviour is unchanged" was
unverifiable. The 3-arg format_path's leaf branch was likewise unpinned while the 2-arg one was;
it formats without the group prefix on purpose, so a later conversion would move results on
disk. Both are now covered.

The README taught the thing this PR replaces. key.params["N"] # ❌ KeyError was presented as
the shape to avoid — which is exactly what param resolves — and neither it nor the new files were
listed. Same for CLAUDE.md.

Still not converted, and the earlier reason was wrong

format_path (both forms) and _validate_path_keys keep their own copies of the rule. The reason
given in the previous commit — that _resolve_name cannot report which way a name resolved — is
false: resolved == pk reports exactly that, and a reviewer reproduced the conversion byte-for-byte.

The real reason to defer is that converting format_path changes its exception type for an
ambiguous leaf, from ErrorException to AmbiguousPathKeyError. That is a public behaviour change,
and it does not belong in a pull request that adds an accessor.

Verification

Full suite 10/10 units, 919 pass, 0 fail, before and after formatting.

`work_fn` opened with `Float64(key.params["system.kbT"])`, and both halves of that were a hazard.

A misspelled name was a bare `KeyError` naming only the string that was wrong. It is raised inside
the work function, which is running on a worker, dispatched by `run!` — so the config that has the
right spelling and the function that has the wrong one are in different files, and nothing compares
them until a point is computed. `param` reports what the key does carry:

    DataKey has no parameter "system.kbt". It carries: model.a, numerics.nsteps, system.a, system.kbT.

The type was the other half. TOML gives `2` as `Int64` and `2.0` as `Float64`, so a config edited
from `[2.0, 2.5]` to `[2, 3]` changed what reached the kernel without changing anything a reader
would look at. `param(key, name, Float64)` converts; `param(key, name, Int)` throws `InexactError`
on a value that is not one, at the boundary rather than two layers down.

The resolution rule is NOT new. `format_path` already resolves a `path_key` exact-then-leaf and
raises `AmbiguousPathKeyError` on a leaf in two groups; `param` uses the same rule and the same
exception, carrying the groups its message asks the caller to disambiguate with. A second rule here
would make `param(key, "a")` and `format_path` disagree about what "a" means.

Suite: 10/10 units, 908 pass, 0 fail.
@github-actions github-actions Bot added the enhancement New feature or request label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📚 Docs preview: https://codes.sota-shimozono.com/ParamIO.jl/previews/PR33/

(updates on each push to this PR)

@codecov

codecov Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

The first version of `param` re-implemented exact-then-leaf resolution instead of reusing it, which
is what the commit message claimed to be doing. The rule was already written out four times — twice
inline in `format_path`, once in `_resolve_axis_value`, once in `_validate_path_keys` — and the
copies had drifted: two raise `AmbiguousPathKeyError` and two raise a bare `error` for the same
condition. Adding a fifth was the opposite of the stated intent.

`_resolve_name(name, available)` is now that rule, once. It returns the KEY rather than the value,
so callers that want the value, the group prefix, or neither all read from one place. `param` is two
lines on top of it, and `_resolve_axis_value` is three.

Two things the first version also had wrong, both visible once it was one expression:

`get(key.params, name, nothing)` used `nothing` as the absent sentinel, so a parameter whose value
really was `nothing` would have been reported as missing. `_resolve_name` asks the key set.

Control flow had three exits at different nesting depths. It has one.

`format_path`'s two inline copies are deliberately NOT converted here. They branch on WHICH way the
name resolved — an exact match formats with the group prefix, a leaf match without it, because "user
chose plain name intentionally" — and `_resolve_name` does not report that. Changing how a path
segment is rendered moves where existing results live, which does not belong in a pull request that
adds an accessor.

Suite: 10/10 units, 908 pass, 0 fail.
Six review passes over this branch. What they found, and what changed.

A bare `catch` around the resolver took `InterruptException` and any bug with the same hand as the
two conditions it meant to allow. The consequence is not abstract: `build_axis_formats` would return
an empty Dict, `format_path` would fall back to fixed2, and 0.006 and 0.008 would both render as
"sysg0.01" — one run's directory silently overwriting another's, which is the failure this file's
own header says auto mode exists to prevent. Fixed by removing the `try`, not by narrowing it:
`_lookup_name` returns the outcome (key / groups / nothing) and `_resolve_name` decides what to
raise, so the caller for which absent and ambiguous are ordinary asks instead of catching.

`param(key, name, T)` promised to refuse a conversion that changes the number. `convert` only does
that for integer targets: it returns 9007199254740992 for a Float64 of 2^53+1, and 2.1f0 for a
Float32 of 2.1, silently. Now converted and checked, so the promise holds for every T. The test
fixture had to be replaced to show it — 2.5 is exact in Float32, so the first version asserted a
refusal on a value that cannot lose anything and passed for the wrong reason.

The version was 0.5.0 for a purely additive change. DataVault pins `ParamIO = "0.3, 0.4"`, so the
next layer of the stack could not have resolved it at all. 0.4.8.

Three comments claimed things that were not true. The header said the rule's four copies "drifted:
two raise AmbiguousPathKeyError and two raise a bare error" — it is one typed, two bare, and one
that raised nothing at all, and erasing that last one hid the larger difference. The catch comment
said this was "the one caller" that swallows; `_safe_format_path` in diagnose.jl does the same. A
testset title claimed parity with the path builder, which raises a different exception for the same
condition.

Two functions the rewrite touched had no test that reached them. Every existing call to
`build_axis_formats` uses fully dotted path_keys, which take the exact-match short-circuit — so the
leaf branch and both failure paths were unexercised, and "behaviour is unchanged" was unverifiable.
The 3-arg `format_path`'s leaf branch was likewise unpinned, while the 2-arg one was: it formats
WITHOUT the group prefix on purpose, so a later conversion would move results on disk.

README taught `key.params["N"] # ❌ KeyError` as the shape to avoid — which is exactly what `param`
now resolves — and listed neither it nor the new files. Same for CLAUDE.md.

Still not converted: `format_path` (both forms) and `_validate_path_keys`. The reason given earlier
was wrong — `resolved == pk` does report which way a name resolved, so they can be converted. The
real reason to defer is that doing so changes `format_path`'s exception type for an ambiguous leaf,
which is a public behaviour change and does not belong in a pull request adding an accessor.

Suite: 10/10 units, 919 pass, 0 fail.
…tower

Testing the conversion on `Int` and `Float64` measured two cells of a table with several interesting
corners, and the guard was wrong in one of them.

A user type with a `convert` method and no `==` falls back to identity comparison, so
`convert(::Type{Celsius}, 3) == 3` is false — the conversion is entirely correct and the guard
refused it anyway. It was reporting "a different object" as "a different value". Measured:

    param(k, "n", Celsius)  ->  InexactError      before
    param(k, "n", Celsius)  ->  Celsius(3.0)      after

The guard now stops at the numeric tower, where `==` between an Int and a Float is exact and the
comparison means what the guard needs it to mean. Outside numbers, `param` gives back whatever
`convert` gives back.

The tests now span the type space rather than the two cells that motivated the feature:
Rational, BigInt, BigFloat, String, Vector{Int} -> Vector{Float64}, Any, Symbol and a user type with
its own convert method. Two of them are there specifically to hold the boundary in place — that an
exact narrowing (2.5 -> Float32) must NOT be refused, or an implementation that refuses everything
would pass; and that `Celsius(3.0) != 3`, which is the fact that makes widening the guard wrong, so
nobody reintroduces it.

Bool's position is pinned too, without being changed: `Bool <: Integer` in Julia, so true converts
to and from the numeric types without complaint. Not a defect to fix here, but a surprise worth
recording.

Suite: 10/10 units, 939 pass, 0 fail.
@sotashimozono
sotashimozono merged commit 8c93475 into main Sep 8, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

work_fn always opens with the same unpacking stanza, and a typo in it fails on a worker

1 participant