Custom rollout depth

[jancervenka]

Draft resolving #88

[BoZenKhaa]

Hi Honza! Thanks for a quick attempt at this! I have a few comments.

First, I like the approach of putting the depth as a parameter of the rollout estimator. I think this is elegant and easy to follow. Though as a consequence, it will be difficult to use the current depth in the tree in the rollouts. At least for me, this is not a big issue and it's simpler this way, but maybe in some use cases, this is needed. What do you think @zsunberg? This could be addressed by still passing the depth to the estimate_value function, and implementing more rollout estimators:

• default rollout estimator, that does not use depth passed to it but instead uses depth defined in SolvedRolloutEstimator
• depth-dependent estimator, that does use depth passed to rollout and replicates the old behavior

Regarding the code, I tried to add commits in your branch that will let the tests run, I haven't figured out how to add them here (do you know how to do that?).

• Added typespec to new SolvedRolloutEstimator jancervenka/MCTS.jl#1 The new in the inner constructor of SolvedRolloutEstimator requires types:

    function SolvedRolloutEstimator(policy, rng, depth::Union{Int, Nothing}=nothing)
        new{typeof(policy), typeof(rng)}(policy, rng, depth)
    end

• Added kwargs in a call to the RolloutSimulator constructor jancervenka/MCTS.jl#2 POMDPs.RolloutSimulator in rollout function needs to be called with keyword arguments since there is no method taking nothing as positional argument:

RolloutSimulator(rng::AbstractRNG, d::Int=typemax(Int)) #not possible to use nothing here for depth
RolloutSimulator(;rng=Random.GLOBAL_RNG, eps=nothing,   max_steps=nothing) # you can pass nothing to this method using kwargs

Also, you can test your code quickly from REPL. Some tests are failing right now, and this is how you run them:

1. launch REPL in the root of the MCTS package
2. turn package mode on in REPL with ']'
3. run activate . to activate the environment
4. run test to run the tests

Alternatively, you can run the runtests.jl file, but the tests may be using a different environment than the package. The above method should handle that.

I think as the next step, you can have a look at the test set, but let's wait for @zsunberg before fixing them.

[jancervenka]

Thank you for the feedback! I will fix the tests once we agree that the proposed solution is OK.

Btw the best way to suggest changes in PR is to create a comment and use the "add a suggestion" feature.

[BoZenKhaa]

These changes allow for runtest.jl to run.

[zsunberg]

@WhiffleFish can you please review these changes and let me know what you think.

[WhiffleFish]

Overall, I think the general approach looks good. However, in my opinion, it still requires a few more changes before we merge.

As a final note, some tests failed with the changes. Line 26 of test/options.jl errored because it still included depth as an argument in the value estimation call, so that should be rectified possibly along with others that may fail once this one doesn't error.

[jancervenka]

Thank you for the review! @BoZenKhaa @WhiffleFish

• I have moved the max_depth attribute and the default constructor to RolloutEstimator and propagated the value to SolvedRolloutEstimator
• max_depth now has a finite default value of max_depth=50. Is it reasonable?
• I have added the eps attribute to RolloutEstimator so it exposes both the max_steps and eps of RolloutSimulator API. The default value is eps=nothing, is that okay or should I change it to eps=0.01?
• The tests are now passing

[jancervenka]

@BoZenKhaa I have added your suggestions. I have not found any inconsistencies in the documenttation.

[BoZenKhaa]

@BoZenKhaa I have added your suggestions. I have not found any inconsistencies in the documenttation.

I meant this

but it's most definitely minor thing.

I very much like the state of the changes!

[zsunberg]

Hi all, sorry for the delayed response - I have been quite busy here. The changes are quite nice. Thank you for being so thorough.

One question to consider here: In this PR you have reduced the arguments for estimate_value from (mdp, s, d) to (mdp, s). I think that, instead, we should keep this argument (in case anyone wants to implement the old behavior), but just ignore it by default.

Any other thoughts?

[zsunberg]

In the docstrings for the solvers, please change the documentation for the depth argument to reflect that it no longer applies to the rollouts.

[zsunberg]

@WhiffleFish are the changes you requested complete?

[BoZenKhaa]

@zsunberg

One question to consider here: In this PR you have reduced the arguments for estimate_value from (mdp, s, d) to (mdp, s). I think that, instead, we should keep this argument (in case anyone wants to implement the old behavior), but just ignore it by default.

Any other thoughts?

We discussed this with @WhiffleFish in the review. The outcome was that making the new behavior default is a breaking change, keeping the legacy functionality would not be generally useful and would unnecessarily complicate things.

If you agree, I think this is ready :-D

[zsunberg]

Hi all, sorry that this fell stale - this semester was very busy for me, but it just ended. I think it is useful to allow the old behavior, so I will go through and add the depth argument back in and fix the other minor issues and merge this tomorrow.

[zsunberg]

Thanks a bunch @jancervenka @BoZenKhaa ! I am sorry that this took so long, but it is a useful addition!