Probabilistic Effects are not applied correctly

[RushangKaria]

Currently, if there are common sets of probabilistic effects, they are not applied correctly since the parser expects the common effects to be defined outside the probabilistic block

Here is a simple example. (assume empty preconditions and a single parameter x).

:effect (and (probabilistic 0.7 (and (not (a ?x)) (b ?x))
                            0.3 (and (not (a ?x)) (c ?x))

[RushangKaria]

(not (a ?x)) is common to both effects and this causes application of the effect to result in invalid states.
Moving this effect to outside the probabilistic block fixes the issue. However, the pddl producing the issue is valid pddl and thus this seems like a simulator bug.

[ronuchit]

hi Rushang! i'm not sure I understand the example you gave. what is wrong with it?
with probability 0.7, we will remove (not (a ?x)) from the state and add (b ?x)
with probability 0.3, we will remove (not (a ?x)) from the state and add (c ?x)

isn't this what is desired? i don't quite get what you mean by "this causes application of the effect to result in invalid states"

[RushangKaria]

Hi Rohan, You are correct that the desired result removes a and adds either one of b or c. However it is not happening!

Using this domain file with this problem file and getting possible successors of the action returns two states with the predicate a not removed in either of them. You can replace the tireworld domain and problem files with this.

(define (domain tireworld)
  (:requirements :typing :strips :probabilistic-effects)
  (:types location)
  (:predicates
        (a)
        (b)
        (c)
       (test)
  )

  ; (:actions test)

  (:action test
    :parameters ()
    :precondition (and (a) (test))
    :effect (and 
                (probabilistic 0.8 (and (not (a)) (b))
                               0.2 (and (not (a)) (c))
  ))
)

Problem

(define (problem tireworld-3)
  (:domain tireworld)
  (:objects
    x - location
  )
  (:init
    (a)

)
  (:goal (and (c)))))

Applying the action test to the initial state and calling get_all_successors() returns two states where the predicate a is not removed but the others are added correctly.

[RushangKaria]

sampling a transition works fine, but trying to get all possible successors does not.
i hope this helps.

[RushangKaria]

the issue is that the predicate a is not being removed (due to the way probabilistic effects are handled). The current handler assumes each probabilistic effect block to have no common effects (in this case (not (a)) is the common effect).

[ronuchit]

hi Rushang, thanks for reporting, i'll need to write a unit test to check this and figure out the bug. will get to it later!

[ronuchit]

hi Rushang, i've successfully recreated the bug in a unit test (c1a0b95)

unfortunately, the logic in _apply_effects is extremely convoluted and in dire need of a careful rewrite. Tom and i don't work on PDDLGym anymore, so it's going to be tough to devote time to fixing this properly. i would recommend just sticking with the version that works for now (you can see in the commit above, the test2 operator works but the test1 operator does not).

in terms of the actual bug, these lines are doing the wrong thing, by adding individual atoms from each effect into the cur_probabilistic_lifted_effects list. but really, we have many regrets about this entire file, and this function in particular probably has other bugs we aren't currently aware of. if you'd like to take a stab at fixing this, of course we'd be happy to merge any contributions.

side note: another bug i noticed is that if you call get_successor_states(), it's still only going to return you the effects under a single (arbitrarily chosen) operator whose preconditions are satisfied, because get_successor_state() calls _select_operator(), which doesn't properly account for the case where it might need to return more than one operator.

sorry that i couldn't be more help, Rushang 😢 cc @tomsilver in case you have time to dig deeper into this

[RushangKaria]

Thanks Rohan!

I've been just using my workaround to move the common effects outside the loop. I'll try to see if i can open up a pull request in case i do manage to fix the issue!

[RushangKaria]

I might have a fix for this. Where can i find the process for creating tests and pull requests? @tomsilver

[fteicht]

@RushangKaria I have just discovered the exact same bug and was thinking of searching for a fix when I read this thread, especially your last message when you say that you found a fix.

I am the maintainer of a fork of pddlgymnasium (which is itself a fork of pddlgym enabling gymnasium instead of gym) for which I build and upload wheels on PyPi.

I have already corrected some annoying parsing bugs, including bugs related to parsing probabilistic effects. I would be happy if you can create a pull request to my repository with your fix of the probabilistic effect application bug. Thanks a lot in advance!

[RushangKaria]

Hi @fteicht ,

Gosh! Its been a long time. I'll have to try and search if I can find the code and fix somewhere! IIRC the fix was not too difficult.
I'll try to respond by next week.

[fteicht]

Hi @RushangKaria I'm glad to hear from you since this thread is nearly one year old!
Thanks so much for doing some archeology in your code projects to find the fix.
I am surprised that the fix is simple, because it seems to me that computing all the next probabilistic states of a given operator in a given state requires a recursive procedure on the tree of nested probabilistic effects of that operator. But I don't see any recursive procedure in the present code, which would require a significant rewriting of the _apply_effects operator IMHO.
Anyway, I am impatient to see your fix!

[RushangKaria]

Hi @fteicht

Im afraid I cannot find the fix! The issue happens with the common effects as I had mentioned earlier. IIRC the fix I made collects such operators iteratively in a set and then applies the rest of the logic as-is. I'm sorry that I couldn't help you much.