POMDPs v0.8 Compatibility #26

MaximeBouton · 2019-09-16T23:32:39Z

The main tweak is for ordered_vector tests.

The build is failing because DiscreteValueIteration needs n_states

rejuvyesh · 2019-09-17T00:18:28Z

Shouldn't it work with deprecation though? We can remove them in the next update? We can remove it from requirements for VI?

rejuvyesh · 2019-09-17T00:21:37Z

src/sparse_tabular.jl

-POMDPs.states(p::SparseTabularProblem) = 1:n_states(p)
-POMDPs.actions(p::SparseTabularProblem) = 1:n_actions(p)
-POMDPs.actions(p::SparseTabularProblem, s::Int64) = [a for a in actions(p) if sum(transition_matrix(p, a)) ≈ n_states(p)]
+POMDPs.states(p::SparseTabularProblem) = 1:size(p.T[1], 1)


Should we call collect on these?

As is, states is an iterator, one can call collect(states(mdp)). I don't think we want to collect by default, what would be the use case?

I don't think we should collect by default

MaximeBouton · 2019-09-17T00:27:42Z

We have not implemented depraction warning in POMDPs.jl for n_actions, n_states yet. I don't know if the plan is to have them in v0.8 or to just remove those functions.

edit: Do you mean deprecation warnings in POMDPModelTools for n_states(::UnderlyingMDP) etc ?

MaximeBouton · 2019-09-17T00:40:10Z

@zsunberg How do we want to deal with the info generation fucntion?

Should the usage be gen(DBNOut(:sp, :r, :info), mdp, s, a, rng)
Does it mean POMDPModelTools needs to add a node in the DDN?

rejuvyesh · 2019-09-17T00:42:12Z

We have not implemented depraction warning in POMDPs.jl for n_actions, n_states yet. I don't know if the plan is to have them in v0.8 or to just remove those functions.

Yeah, since it was part of the API, we should have added the deprecation.

zsunberg · 2019-09-17T02:08:40Z

Shouldn't it work with deprecation though? We can remove them in the next update? We can remove it from requirements for VI?

We have not implemented depraction warning in POMDPs.jl for n_actions, n_states yet. I don't know if the plan is to have them in v0.8 or to just remove those functions.

The deprecation warnings are implemented in src/deprecated.jl in v0.8. They are not deprecated in 0.7.3, which means we should just keep n_states(::UnderlyingMDP) for now. We can remove it later.

We're just trying to get all the packages to work with 0.8 at this stage, not cleaning them up yet. If we have both n_states and length, then it will work with both 0.7.3 and 0.8, which is what we want at the moment.

zsunberg · 2019-09-17T02:14:00Z

@zsunberg How do we want to deal with the info generation fucntion?

I was thinking like this: https://github.com/JuliaPOMDP/POMDPs.jl/blob/146e97f5bc8e59f3e50a40aa608d8470fd571e43/test/test_ddn_struct.jl#L22
in lines 22-31

POMDPModelTools will have a function called add_infonode or something, and if people want an info output for their problem, they will have to add it with

POMDPs.DDNStructure(::Type{MyPOMDP}) = pomdp_ddn() |> add_infonode

Does that seem reasonable? The other option would be to overwrite

POMDPs.DDNStructure(::Type{M}) where M<:POMDP

or

pomdp_ddn()

but that seems like a very bad idea (and might even produce a warning)

MaximeBouton · 2019-09-17T17:13:46Z

the piping approach sounds better, do you expect this in this PR or later?

zsunberg · 2019-09-17T17:15:52Z

Yeah, I think it should be in this one. I think it should be easy to implement, right? I can do it if you would have me implement it.

MaximeBouton · 2019-09-17T17:19:03Z

is that just it:
add_infonode(ddn) = add_node(ddn, :info, ConstantDDNNode(nothing), (:s, :a))
?
You can do it @zsunberg if you can I still have some misunderstandings:
What happens to generate_sri and generate_sori ?

zsunberg · 2019-09-17T17:25:53Z

Ok, sure I'll do it. Does anyone have a preference on whether this node should be called :info or i? I guess I am leaning towards just i.

MaximeBouton · 2019-09-17T17:26:47Z

I prefer info, I think it is clearer and not too verbose.

zsunberg · 2019-09-17T17:29:13Z

Ok, in that case, should I update POMDPSimulators to use `:actioninfo`, and `:updateinfo` instead of `ai` and `ui` in histories?

…

On Tue, Sep 17, 2019 at 10:26 AM Maxime ***@***.***> wrote: I prefer info, I think it is clearer and not too verbose. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#26?email_source=notifications&email_token=ABALI24QITEWHP3FSDTCCLTQKEHNRA5CNFSM4IXIOB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD65I33I#issuecomment-532319725>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABALI2477YIGNBV3QNRRBMLQKEHNRANCNFSM4IXIOB2Q> .

MaximeBouton · 2019-09-17T17:30:51Z

Do you think it is too long to use :actioninfo ? If not then let's do it for consistency.

zsunberg · 2019-09-17T17:34:13Z

for (s, a, r, sp, i, ai) in stepthrough(m, policy, "s, a, r, sp, info, actioninfo")

vs

for (s, a, r, sp, i, ai) in stepthrough(m, policy, "s, a, r, sp, i, ai")

Yeah, it doesn't seem too long to me. I think we should lengthen both of them.

zsunberg · 2019-09-17T17:36:12Z

What happens to generate_sri and generate_sori

Use gen(DDNOut(:sp, :r, :info), ...) and gen(DDNOut(:sp, :o, :r, :info), ...).

zsunberg · 2019-09-18T23:58:14Z

Hey @MaximeBouton , I added the info stuff, but I did not update the docs yet. I will have that completed shortly. Tests pass for POMDPs v0.7.3, but not v0.8.0 - there is still some stuff to fix

MaximeBouton

Thanks @zsunberg, I just have two comments (see above) and then we can merge

MaximeBouton · 2019-09-19T00:50:13Z

src/info.jl

+###############################################################
+
+
+if DDNStructure(MDP) isa POMDPs.DDNStructureV7


shouldn't we check on the POMDPs.jl version instead?

That's what I initially though, but it didn't seem like there was a good way to do that, so this seems like a reliable enough proxy

if Pkg.installed()["POMDPs"] < v"0.8.0"
something like that?

I don't know how to reply to your comment on this below.

The problem with Pkg.installed() is that it can take a really long time (or at least it could in the past)

and it adds Pkg as a dependency (which is probably fine)

MaximeBouton · 2019-09-19T00:50:57Z

src/info.jl

+"""
+    add_infonode(ddn::DDNStructure)
+
+Create a new DDNStructure object with a new node labeled :info with parents :s and :a


I think it would be valuable to have the example from the conversation in here

👍 it is in the docstring now

zsunberg · 2019-09-19T01:33:39Z

Thanks @zsunberg, I just have two comments (see above) and then we can merge

We still need to fix some stuff with generate_ and sampletype so the tests pass with POMDPs v0.8. I am working on that now

MaximeBouton · 2019-09-19T01:43:17Z

test/test_underlying_mdp.jl

@@ -11,7 +11,7 @@ let

    solver = ValueIterationSolver(max_iterations = 100)
    mdp_policy = solve(solver, mdp)
-    pomdp_policy = solve(solver, pomdp)
+    pomdp_policy = solve(solver, UnderlyingMDP(pomdp))


This test does not make sense anymore

Yeah, but it doesn't hurt anything. At least it exercises a lot of the functions. I think it is fine

MaximeBouton · 2019-09-19T01:48:28Z

src/info.jl

+###############################################################
+
+
+if DDNStructure(MDP) isa POMDPs.DDNStructureV7


if Pkg.installed()["POMDPs"] < v"0.8.0"
something like that?

zsunberg · 2019-09-19T02:32:35Z

I cleaned up things so that tests pass with POMDPs v0.8, but then noticed some problems with FullyObservablePOMDP, so I'm in the process of fixing it, but I won't have a chance to finish until tomorrow

zsunberg · 2019-09-20T00:16:27Z

Ok, I think this is ready now. Tests pass with POMDPs 0.7.3 and 0.8. @MaximeBouton any other comments?

removed n_states, n_actions, ...

545fd3e

MaximeBouton mentioned this pull request Sep 16, 2019

Checklist for upgrading packages to work with v0.8 JuliaPOMDP/POMDPs.jl#259

Closed

27 tasks

rejuvyesh reviewed Sep 17, 2019

View reviewed changes

MaximeBouton mentioned this pull request Sep 17, 2019

POMDPs v0.8 Compatibility JuliaPOMDP/DeepQLearning.jl#22

Merged

restore n_states, n_actions

7ee8c29

added info update (no docs yet)

d621206

zsunberg added 3 commits September 18, 2019 17:03

only make value iteration solve an mdp

5d007da

updating docs

3052882

Merge branch 'master' into pomdps-08-compat

3fc9564

MaximeBouton commented Sep 19, 2019

View reviewed changes

added docs for info

d5f2579

MaximeBouton commented Sep 19, 2019

View reviewed changes

cleaned up some stuff for 0.8 compat, but not ready yet

fd95118

tests all pass with both POMDPs 0.7.3 and 0.8

56c6cdb

don't test for inferrence in GenerativeBeliefMDP in Julia 1.0

44cf4cd

zsunberg merged commit 1e4096e into master Sep 20, 2019

zsunberg deleted the pomdps-08-compat branch August 5, 2020 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POMDPs v0.8 Compatibility #26

POMDPs v0.8 Compatibility #26

MaximeBouton commented Sep 16, 2019

rejuvyesh commented Sep 17, 2019 •

edited

rejuvyesh Sep 17, 2019

MaximeBouton Sep 17, 2019

zsunberg Sep 17, 2019

MaximeBouton commented Sep 17, 2019 •

edited

MaximeBouton commented Sep 17, 2019

rejuvyesh commented Sep 17, 2019

zsunberg commented Sep 17, 2019

zsunberg commented Sep 17, 2019

MaximeBouton commented Sep 17, 2019

zsunberg commented Sep 17, 2019

MaximeBouton commented Sep 17, 2019

zsunberg commented Sep 17, 2019

MaximeBouton commented Sep 17, 2019

zsunberg commented Sep 17, 2019 via email

MaximeBouton commented Sep 17, 2019

zsunberg commented Sep 17, 2019

zsunberg commented Sep 17, 2019

zsunberg commented Sep 18, 2019

MaximeBouton left a comment

MaximeBouton Sep 19, 2019

zsunberg Sep 19, 2019

MaximeBouton Sep 19, 2019

zsunberg Sep 19, 2019

zsunberg Sep 20, 2019

MaximeBouton Sep 19, 2019

zsunberg Sep 19, 2019

zsunberg commented Sep 19, 2019

MaximeBouton Sep 19, 2019

zsunberg Sep 19, 2019

MaximeBouton Sep 19, 2019

zsunberg commented Sep 19, 2019

zsunberg commented Sep 20, 2019

		###############################################################


		if DDNStructure(MDP) isa POMDPs.DDNStructureV7

POMDPs v0.8 Compatibility #26

POMDPs v0.8 Compatibility #26

Conversation

MaximeBouton commented Sep 16, 2019

rejuvyesh commented Sep 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaximeBouton commented Sep 17, 2019 • edited

MaximeBouton commented Sep 17, 2019

rejuvyesh commented Sep 17, 2019

zsunberg commented Sep 17, 2019

zsunberg commented Sep 17, 2019

MaximeBouton commented Sep 17, 2019

zsunberg commented Sep 17, 2019

MaximeBouton commented Sep 17, 2019

zsunberg commented Sep 17, 2019

MaximeBouton commented Sep 17, 2019

zsunberg commented Sep 17, 2019 via email

MaximeBouton commented Sep 17, 2019

zsunberg commented Sep 17, 2019

zsunberg commented Sep 17, 2019

zsunberg commented Sep 18, 2019

MaximeBouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsunberg commented Sep 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsunberg commented Sep 19, 2019

zsunberg commented Sep 20, 2019

rejuvyesh commented Sep 17, 2019 •

edited

MaximeBouton commented Sep 17, 2019 •

edited