Learning-and-Intelligent-Systems / predicators

Learning for effective and efficient bilevel planning
MIT License
84 stars 14 forks source link

Some Suggestions to the Satellite Domain #1687

Closed Jaraxxus-Me closed 5 days ago

Jaraxxus-Me commented 2 weeks ago

Hi, thanks for this library. After carefully studying your implementation, I have some questions and potential suggestions for the Satellite Domain. Maybe I misunderstood something, correct me if I'm wrong.

Currently, the operator "MoveTo" has no pre-conditions and only creates one "Sees", deleting nothing. See here. This is a bit problematic in the continuous space:

  1. If we want to achieve Markov property in the low-level states, the actual state should include past trajectories or add more information. Since the objects that have been seen by the satellite will be "remembered". This means the current best action to take also depends on if we have seen some objects before, while the low-level states currently do not provide such information. We just know where we are, but don't know where we have been.
  2. This exist a bit of ambiguity in terms of the "Sees" predicate. See the attached visualization. If I execute MoveTo(sat0, obj1), sat0 may also see obj0, even though the high-level operator is not adding Sees(sat0, obj0). In this case, the atoms in high-level planning and the actually low-level states are a bit separated. I see there exist "ingore_effects", but unsure what will happen if Sees appear in both "add_effect" and "ignore_effect". Screenshot 2024-07-03 at 10 27 12 AM

To solve these, I suggest some modifications to the domain definition:

  1. Make Sees much harder (sat should be close enough to obj, and the objects are initially far away from each other enough), so one "MoveTo" strictly only adds one actual "Sees".
  2. Add one more predicate called "ViewClear(?sat)". And one more operator "MoveAway(?sat, ?obj)". Change the "MoveTo(?sat, ?obj)" def: pre-cond: {ViewClear(?sat)} Effect+: Sees(?sat, ?obj) Effect-: ViewClear(?sat, ?obj) MoveAway(?sat, ?obj) def: pre-cond: Sees(?sat, ?obj) Effect+: ViewClear(?sat) Effect-: Sees(?sat, ?obj)

    And also add the MoveAway() sampler. In this case, we don't need to have ignore_effects, and the low-level states achieve Markov Property.

As I'm actively working on this, I can do these and submit a pull request if needed.

NishanthJKumar commented 1 week ago

Hi Bowen,

Thanks for the question/comments. The points you raised are actually all intentional.

  1. On the lack of the markov property for the low-level state: We do not assume the low-level state is Markovian: our approaches are able to handle such cases by sharing decision making between the low-level and the abstract level. However, I do understand that a pure RL approach over the low-level state (which is maybe what you're working on) would require it to be Markovian.
  2. On the discrepancy between the low-level and high-level: you are absolutely right that there is some 'disagreement' between the low and high levels here. In general, this is expected: the low-level state might actually have more predicates true than the predicted high-level state. As explained in our paper, Section 3 paragraph 2, we assume that the atoms true in the actual low-level state are a superset of the atoms predicted by the high-level plan. In this way, the high-level model serves to guide low-level planning and thus does not have to be perfectly correct.

Given this, I'd rather you not modify the original domain. However, if you'd like to subclass the domain and introduce a new domain (maybe called satellites-markov?), I would be happy to review your PR.

Jaraxxus-Me commented 1 week ago

I see. I appreciate these clarifications. Yes, sure, I can introduce the new domain with a PR.

Jaraxxus-Me commented 5 days ago

PR opened.