Repository navigation
feat: param(key, name[, T]) — closes #32 - #33
Merged
Merged
Conversation
`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.
Contributor
|
📚 Docs preview: https://codes.sota-shimozono.com/ParamIO.jl/previews/PR33/ (updates on each push to this PR) |
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.
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.
Closes #32.
work_fnopened withFloat64(key.params["system.kbT"]), and both halves of that were a hazard.A misspelled name was a bare
KeyErrornaming only the string that was wrong — raised inside thework function, on a worker, dispatched by
run!. The config with the right spelling and thefunction 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
catchwas swallowing everything._resolve_axis_valuecaught the resolver's twointended errors and also
InterruptException, and any future bug. The consequence is concrete:build_axis_formatsreturns an emptyDict,format_pathfalls back to fixed2, and0.006and0.008both render assysg0.01— one run's directory overwriting another's, which is the failureformat.jl's own header says auto mode exists to prevent.Fixed by removing the
tryrather than narrowing it._lookup_namereturns the outcome — key,groups, or nothing — and
_resolve_namedecides what to raise. The caller for which absent andambiguous are ordinary now asks instead of catching.
param(key, name, T)did not keep its own promise. It said it refuses a conversion that changesthe number;
convertonly does that for integer targets. Measured:Now converted and checked, so the promise holds for every
T. The test fixture had to be replacedto show it:
2.5is exact inFloat32, so the first version asserted a refusal on a value thatcannot lose anything and would have passed for the wrong reason.
The version was wrong.
0.5.0for a purely additive change, whileDataVaultpinsParamIO = "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
AmbiguousPathKeyErrorand two raise a bareerror". It is one typed, two bare, and onethat 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_pathindiagnose.jldoes thesame. 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_formatspasses fully dottedpath_keys, which take the exact-match short-circuit — sothe 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"] # ❌ KeyErrorwas presented asthe shape to avoid — which is exactly what
paramresolves — and neither it nor the new files werelisted. Same for
CLAUDE.md.Still not converted, and the earlier reason was wrong
format_path(both forms) and_validate_path_keyskeep their own copies of the rule. The reasongiven in the previous commit — that
_resolve_namecannot report which way a name resolved — isfalse:
resolved == pkreports exactly that, and a reviewer reproduced the conversion byte-for-byte.The real reason to defer is that converting
format_pathchanges its exception type for anambiguous leaf, from
ErrorExceptiontoAmbiguousPathKeyError. 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.