Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Nothing from RolloutEstimator parameters #103

Conversation

johannes-fischer
Copy link
Contributor

JuliaPOMDP/POMDPs.jl#441 broke MCTS.RolloutEstimator default parameters or if nothing is selected for eps or max_depth. To align MCTS.jl with JuliaPOMDP/POMDPs.jl#441, nothing should also not be allowed in MCTS.RolloutEstimator.

@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.09 🎉

Comparison is base (dd3531b) 86.62% compared to head (ed88999) 86.72%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   86.62%   86.72%   +0.09%     
==========================================
  Files          10       11       +1     
  Lines         486      482       -4     
==========================================
- Hits          421      418       -3     
+ Misses         65       64       -1     
Impacted Files Coverage Δ
src/domain_knowledge.jl 100.00% <ø> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BoZenKhaa
Copy link
Contributor

Thanks for opening this; I made the change you are reverting with this PR; sorry it caused you issues. The problem with allowing nothing was that it was causing dynamic dispatch in the inner loop of MCTS; that's why I removed it, and reintroducing it back would cause the same issue again.

Please explain in more detail what this change broke with your use case. There may be another way to do this.

To align MCTS.jl with JuliaPOMDP/POMDPs.jl#441, nothing should also not be allowed in MCTS.RolloutEstimator.

What do you mean by this? Could you elaborate?

@BoZenKhaa BoZenKhaa self-assigned this May 10, 2023
@johannes-fischer
Copy link
Contributor Author

This PR is not reintroducing nothing. It is removing nothing from MCTS.RolloutEstimator, too (your PR was in POMDPTools.jl). The goal is to make MCTS.RolloutEstimator work with your changes in RolloutSimulator.

@zsunberg
Copy link
Member

Thanks @johannes-fischer Looks great!

@zsunberg zsunberg merged commit 15302d3 into JuliaPOMDP:master May 11, 2023
6 checks passed
@zsunberg
Copy link
Member

@johannes-fischer should we tag a new version now?

@johannes-fischer
Copy link
Contributor Author

I think at least a new version should be tagged before a new version of POMDPTools is tagged (POMDPTools v0.1.3 does not contain JuliaPOMDP/POMDPs.jl#441 yet, so currently the tagged versions still work just fine together). But might as well do it now before it is forgotten.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants