feat!: Implement AbstractPolicy type hierarchy - #108
Conversation
BatyLeo
changed the title
BatyLeo
left a comment
BatyLeo
left a comment
There was a problem hiding this comment.
Nice PR, I just have a few minor comments and suggestions, otherwise it's great!
| callable in `Policy{B}` makes it an [`AbstractRolloutPolicy`](@ref)`{B}`, which enables | ||
| per-benchmark specialization of [`single_rollout!`](@ref). | ||
| """ | ||
| struct Policy{B,P} <: AbstractRolloutPolicy{B} |
There was a problem hiding this comment.
issue: This is not necessarily a dynamic policy, we then may need two different policy wrappers, once for stochastic policies, and one for dynamic ones (there are no static policies for now in the benchmarks).
There was a problem hiding this comment.
I completely agree, and I was thinking about that while coding the PR.
Should we make a following PR that implements policies for static benchmarks ?
This would unify everything and make it easier for metrics too (see #109)
There was a problem hiding this comment.
I created a ScenarioPolicy and kept the Policy structure for dynamic benchmarks in b0d8396, should we update the naming of Policy too ?
There was a problem hiding this comment.
I think that's fine, at some point we may want to get rid of these Policy and ScenarioPolicy wrappers, which are not very useful in practice
| features, state = observe(env) | ||
| state_copy = deepcopy(state) | ||
| reward = step!(env, y, rng) | ||
| return DataSample(; x=features, y=y, instance=state_copy, extra=(; reward)) |
There was a problem hiding this comment.
question: Do we want to keep the instance field name for now and rename it to state in a future PR?
There was a problem hiding this comment.
For now, I think we can keep it and address it in a following PR.
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
BatyLeo
left a comment
BatyLeo
left a comment
There was a problem hiding this comment.
Here are some last few comments.
In addition to the comments below, now that we have defined the AbstractDynamicPolicy alias, we use it to type evaluate_policy! policy arg:
function evaluate_policy!(policy::DynamicPolicyAfter these few comments, I think we're good.
| return nothing | ||
| end | ||
| """ | ||
| """scen |
2 participants
This pull request addresses the issues raised in: #93 and #106
This PR introduces a new policy hierarchy based on an abstract
AbstractPolicytype with two subtypes:AbstractRolloutPolicy: policies that produce a decision from the current state at each step of a rollout.AbstractFullHorizonPolicy: policies that, given an initial state and a planning horizon, compute the complete sequence of decisions in advance. This is particularly useful for anticipative or offline planning policies.Benefits:
single_rollout!method, allowing users to choose which fields should be recorded in the resultingDataSample.generate_samplecurrently has an unstable interface because it requires atarget_policythat can benothingwhile otherwise being expected to be callable with specific arguments. The new policy types makes it possible to refactor this API into a cleaner and more type-stable design.Notes: To keep this PR focused and make the changes incremental, the existing
Policystruct has been left unchanged. It could be revisited.