feat(strategies): non-throwing parameter(T, default) accessor - #521
Merged
Conversation
Strategies.parameter(T) throws NotImplemented by default, same as id and metadata. But "no parameter" is a legitimate, already-established answer (nothing) for parameter specifically, not an oversight the way a missing id or metadata would be. The real gap is callers that query parameter on a strategy type they did not author and cannot guarantee has overridden the (optional) contract - they had to catch NotImplemented themselves, as OptimalControl currently does by hand in helpers/print.jl. Adds parameter(strategy_type, default), the non-throwing counterpart to parameter(strategy_type) - the same relationship as get(dict, key, default) to dict[key]. default is only substituted when the underlying call throws NotImplemented; a strategy that legitimately implements parameter and returns nothing still returns nothing, not default, and any other exception still propagates. Implemented as a single get-style 2-arg method rather than the issue's alternative keyword-based shape (parameter(T; strict=false)): Julia resolves keyword arguments only after dispatching on positional types, against whichever method that dispatch selects. Every currently-implemented strategy overrides parameter with its own plain 1-arg method with no keyword declared, so the keyword form would throw "no keyword argument strict" for every strategy that already implements the contract correctly, and only work for the ones that don't. The 2-arg method has no such issue: it wraps a call to parameter(T) internally and works uniformly regardless of which concrete method that inner call resolves to. Fixes #518 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Strategies.parameter(strategy_type, default), the non-throwing counterpart toparameter(strategy_type)— the same relationship asget(dict, key, default)todict[key].defaultis only substituted when the underlying call throwsExceptions.NotImplemented; a strategy that legitimately implementsparameterand returnsnothingstill returnsnothing, notdefault, and any other exception still propagates unchanged.AbstractStrategyit did not author (and cannot guarantee implements the optionalparametercontract) query it safely, instead of hand-rollingtry/catch e; e isa NotImplemented || rethrow(); nothing end— which is exactly whatOptimalControldoes today inhelpers/print.jl.Fixes #518
On the issue's two proposed shapes
The issue offered
parameter(T, default)orparameter(T; strict::Bool=true), "either works". They're not actually equivalent in Julia: keyword arguments are resolved only after dispatch on positional types, against whichever method that dispatch selects. Every currently-implemented strategy overridesparameterwith its own plain 1-arg method (no keyword declared), so the keyword form would throwMethodError: no keyword argument strictfor every strategy that already implements the contract correctly, and only work for the ones that fall through to the abstract default (the ones that don't need the safety net in the first place). The 2-arg method has no such issue — it wraps a call toparameter(T)internally and works uniformly regardless of which concrete method that inner call resolves to. Went with the 2-arg form for this reason.Test plan
test/suite/strategies/test_abstract_strategy.jl:defaultwhenparameterisn't implemented at allnothingis not conflated with the "unimplemented" caseNotImplementedexception still propagates (not swallowed)suite/strategies/test_abstract_strategy.jl(90 tests) green🤖 Generated with Claude Code