Remove !?

[mykelk]

Should we remove the ! functions? So, for example, instead of:

transition!(distribution, pomdp::POMDP, state::State, action::Action)

we would have

transition(pomdp::POMDP, state::State, action::Action; distribution = create_transition_distribution(pomdp))

or something like that? It would return distribution. Is there a performance difference? The benefit of this change is that it allows you to have simpler calls if you want the distribution to be allocated for you. I think this would also allow you to work with immutables if you wanted. What do folks think?

[ebalaban]

I like this (the fewer functions, the better). The only potential downside I can [vaguely] think of right now is if, somehow, 'distribution' needs to be preprocessed (initialized) to work properly inside 'transition'. We may not have a straightforward way of knowing whether that was already done on the externally-supplied 'distribution' or not done if 'distribution' was allocated automatically during the call to 'transition'. There should be ways of avoiding this ambiguity, however.

On 8/26/2015 10:08 PM, Mykel Kochenderfer wrote:

Should we remove the ! functions? So, for example, instead of:

transition!(distribution, pomdp::POMDP, state::State, action::Action)

we would have

transition(pomdp::POMDP, state::State, action::Action; distribution= create_transition_distribution(pomdp))

or something like that? It would return |distribution|. Is there a performance difference? The benefit of this change is that it allows you to have simpler calls if you want the distribution to be allocated for you. I think this would also allow you to work with immutables if you wanted. What do folks think?

— Reply to this email directly or view it on GitHub https://github.com/sisl/POMDPs.jl/issues/23.

[mykelk]

Can you think of a scenario where you would need to know whether it was externally supplied vs. allocated automatically?

[etotheipluspi]

If we assume the model is Markovian, we shouldn't need to pre-process the transition distribution (it should only depend on the current state-action pair). Any extra variables, etc that are needed to compute a distribution can be hidden in the POMDP type.

I like this, and I think we should adopt it. I'll benchmark it this weekend, and we should make a decision to adopt it or not sometime next week.

[ebalaban]

Max, I should have made it more clear that I meant my comment to apply to any function that uses this pattern, not just 'transition'. It's hard to foresee right now when such pre-processing would be needed, if at all, so I think we can go ahead with the change and deal with any issues if/when they arise. I was just trying to think of scenarios where there might be a downside to this style of function calls.

On 8/27/2015 8:59 AM, Maxim Egorov wrote:

If we assume the model is Markovian, we shouldn't need to pre-process the transition distribution (it should only depend on the current state-action pair). Any extra variables, etc that are needed to compute a distribution can be hidden in the POMDP type.

I like this, and I think we should adopt it.

— Reply to this email directly or view it on GitHub https://github.com/sisl/POMDPs.jl/issues/23#issuecomment-135477975.

[etotheipluspi]

I did some quick benchmarking on the sniper problem to see the performance difference in using a key worded input for distribution.

I call transition for every state-action combination in my problem (160,000 states and 9 actions).

• Test 1: transition(pomdp, s, a) ~ 4.52 seconds (distribution is not pre-allocated and is created at each call to transition)
• Test 2: transition(pomdp, s, a, d=d) ~1.72 seconds (the distribution d is preallocated but is keyworded)
• Test 3: transition!(d, pomdp, s, a) ~ 1.32 seconds (distribution d is pre-allocated and is a required input arg)

I ran each test 5 times and averaged the time results. It seems like key wording the distribution does have some performance penalty but not much.

I think the penalty is acceptable, and we should proceed with removing the ! notation. What does everyone else think?

EDIT: Tim had the suggestion of trying out the following function form:

transition(pomdp::POMDP, state::State, action::Action, distribution=create_transition_distribution(pomdp))

The distribution is an optional argument. This has the same performance as the transition!() form.

[mykelk]

Good call Tim. I'm good with this. Why is there a penalty for optional arguments rather than default arguments?

[etotheipluspi]

My guess is that the function form suggested by Tim (distribution separated by a ,), compiles to two different functions, while the function form considered previously (distribution separated by a ;) compiles down to a single function.

A simple test:

function test(x::Int64, y::Int64=10)
    println("$x, $y")
end

Here both test(x::Int64) and test(x::Int64, y::Int64) exist in the namespace.

function test(x::Int64; y::Int64=10)
    println("$x, $y")
end

Here onlytest(x::Int64) seems to show up in the namespace.

Maybe there is an additional overhead to compiling the second test() function when the key-worded argument y is used.

[mykelk]

Ah, okay. I have a medium preference for Tim's way, otherwise we can go with the ;.

[etotheipluspi]

I like Tim's way as well.

[mykelk]

Let's do it then. :-) And then close the issue. I assigned this to Max, but someone else is welcome to change the assignment.

[zsunberg]

I'm going to start doing this in like 15 minutes. Watch out.

[zsunberg]

Made the changes. I'm leaving the issue open for now. Is everyone ok with how it looks?

See also #26

Also have a look at simulate() (while it's still there :)). I think it was clearer before the change. The old way was conventional; the new way takes a moment to understand. The line

b = belief(pomdp, b, a, o, b)

seems especially weird.

I'm feeling worse about this after implementing it, but maybe it is better. Someone else can close the issue if they feel especially good about it.

[zsunberg]

Also, the new way just might be a little weird for new problem-implementers. With the old syntax, if you knew what ! meant, it was instantly clear what is going on.

Is the occasional convenience that solver-writers will gain with the new scheme worth the extra few minutes that it will take problem-writers to understand?

[zsunberg]

Ok, after a weekend of letting this settle in, I feel a little better about it. Two questions:

1) Do we want to do this for actions!, observations!, and solve!as well as the changes that have already been made? 2) is there a better name for the preallocated memory argument?

[mykelk]

I think yes for 1. Do you have ideas for 2?

[zsunberg]

I don't have good ideas for 2 unfortunately. preallocated_distribution is what I would want because it better explains what the argument is, but that is probably too long.

[mykelk]

Yeah, that would be goofy.

[zsunberg]

Ok, I took care of the remaining functions. The one weird thing is solve(). There is no create_policy(), so I just initialized the policy to nothing by default. Is that OK? or should we add a create_policy method?

[mykelk]

Yeah, I vote for adding create_policy.

[ebalaban]

Agreed. Also, should we have create_action in addition to create_state and create_observation?

On 9/7/2015 4:22 PM, Mykel Kochenderfer wrote:

Yeah, I vote for adding |create_policy|.

— Reply to this email directly or view it on GitHub https://github.com/sisl/POMDPs.jl/issues/23#issuecomment-138392278.

[zsunberg]

Yeah, for the sake of uniformity, we should probably have action behave like all the other functions

[ebalaban]

It's also nice to have an action object pre-allocated.

On 9/7/2015 4:31 PM, zsunberg wrote:

Yeah, for the sake of uniformity, we should probably have action behave like all the other functions

— Reply to this email directly or view it on GitHub https://github.com/sisl/POMDPs.jl/issues/23#issuecomment-138392803.

[zsunberg]

I'm going to go ahead and make those two changes

[zsunberg]

Added create_policy. update to action() is more complicated, so I opened #28 . I think we're done here. Going to close this.

Everyone have fun fixing all your code! ;)