Fix wrong answers in the task model and heuristics

AGENTS.md

@@ -115,8 +115,9 @@ Everything `compile.py` introduces is named with a leading `__`, and grounding
 collects those into `Task.synthetic` so `Task.visible_plan` can hide them. Two
 traps worth knowing:
 
-- **Synthetic actions must declare a zero cost.** Grounding charges 1 for any
-  action with no cost effect, which silently inflates the metric — closing a
+- **Synthetic actions must declare a zero cost.** Without a `(total-cost)`
+  metric, grounding charges 1 for any action with no cost effect (under one, 0,
+  as IPC planners do), which would silently inflate the metric — closing a
   plan or observing a constraint is bookkeeping, not work.
 - **Forced vs optional monitors.** `sometime` can use an action the planner
   applies when convenient; `at-most-once` and `sometime-after` cannot, because
@@ -162,8 +163,18 @@ planners must go through the task rather than the operator to honour them:
 - **Conditional effects (`flip`, `elevator`)**: the delete-relaxation heuristics
   (`hadd`, `hff`, `lmcut`, `h^m`) are *not guaranteed admissible* on domains with
   conditional effects because each conditional effect is relaxed into its own
-  operator. For guaranteed-optimal plans there use `bfs`, `dijkstra`, or
-  `astar`/`idastar` with the `blind` heuristic. Optimality tests use these.
+  operator. `lmcut` and `h2` clear their `admissible` flag on such tasks. For
+  guaranteed-optimal plans there use `dijkstra`, or `astar`/`idastar` with the
+  `blind` heuristic. Optimality tests use these.
+- **Fact ids are assigned in sorted order** and every id set is built sorted
+  (`grounding._ids`). A set of ints iterates in insertion order where ids share
+  a hash slot, so filling one while walking atoms in hash order made LM-cut's
+  tie-breaks — and so expansions and plans — depend on `PYTHONHASHSEED`.
+  `tests/test_semantics.py` runs a search under three seeds to pin this.
+- **`validate_plan` replays through `Task.apply`**, so it cannot catch a
+  semantics bug in the task itself. `tests/test_semantics.py` pins semantics
+  on tiny hand-written tasks whose answer is obvious by inspection — add a case
+  there when you change grounding, axioms, complements or costs.
 - **Matplotlib is optional**: only `jupyddl.viz` and `jupyddl.benchmark.plot_summary`
   need it; run headless with `MPLBACKEND=Agg` if no display (`jupyddl.viz` already
   forces the Agg backend).

CHANGELOG.md

@@ -55,6 +55,44 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   `.github/` or `docs/`, so filing them under `.docs/` would have silently
   dropped the contributing link on the issue and pull-request forms.
 
+### Fixed
+Wrong answers from the task model and the heuristics. Each of these returned a
+plan, a cost or a value without complaint, and `validate_plan` accepted the
+plans because it replays them through the same model. They are now pinned by
+`tests/test_semantics.py` on tiny tasks whose right answer is obvious.
+
+- **Derived facts were never retracted.** An action that deleted the support of
+  a derived fact left it true, and `(not <derived>)` held from the initial state
+  on. Axioms are now recomputed from scratch after every step, and a negated
+  derived fact is maintained with them. Negating a derived predicate *inside
+  another rule* needs stratification and is now refused by name.
+- **An atom both added and deleted left its complement true as well**, so
+  `(not (at a))` and `(at a)` could hold together, and a `when` that re-added a
+  deleted atom did the same. For complement facts the delete now wins.
+- **Action costs.** Under a `(total-cost)` metric an action with no cost effect
+  now costs 0, as in IPC planners, rather than 1 — on pegsol and sokoban the
+  "optimal" planners were optimising the wrong objective. A cost inside `when`
+  was charged whether or not the condition held; it is now refused, as is a
+  negative cost.
+- **A numeric fluent that is never defined read as 0**, so `(distance a c)` for
+  a road that does not exist cost nothing. Conditions and effects reading one
+  now fail, as PDDL 2.1 specifies.
+- **`blind` was not admissible** when any action is free, which every PDDL 3
+  compilation introduces, so `astar`+`blind` could return a costlier plan.
+- **`h1`/`h2`/`hm` crashed on every numeric or timed task**, and **LM-cut
+  crashed on an empty goal**, which made the default `solve()` crash too.
+- **`lmcut` and `h2` claimed admissibility on tasks with conditional effects**,
+  where they can overestimate. The instance flag now says so.
+- **`solve_task(task, "astar")` used `hff`**, which is not admissible, so the
+  default A* was not optimal. Optimal planners now default to `lmcut`. `-H none`
+  also fell through to `hff`; it now means `blind`.
+- **`make_heuristic(LMCutHeuristic, task)` returned the class unbuilt**, and the
+  planner then crashed calling it on a state. Classes and partials are now built.
+- **Results depended on `PYTHONHASHSEED`.** Fact ids followed string hashing,
+  so LM-cut broke ties differently in every process: gripper with `astar`/`lmcut`
+  expanded 1725, 1758 or 1760 nodes and returned different plans. Ids are now
+  assigned in sorted order, so a benchmark reproduces.
+
 ## [2.4.0]
 
 ### Added

README.md

@@ -505,9 +505,9 @@ jupyddl requirements --verbose  # with the full compilation notes
 
 | Requirement | PDDL | Support | What jupyddl does |
 |---|---|---|---|
-| `:action-costs` | 3.0 | **native** | `(increase (total-cost) k)` and metric minimisation. Operator costs feed straight into g, so A*/Dijkstra optimise cost. |
+| `:action-costs` | 3.0 | **native** | `(increase (total-cost) k)` and metric minimisation. Operator costs feed straight into g, so A*/Dijkstra optimise cost. Under a metric over `(total-cost)` an action with no cost effect is free, as in IPC planners; without a metric every action costs 1. A cost inside `when` and a negative cost are refused. |
 | `:conditional-effects` | 1.2 | **native** | `when` and `forall` inside effects. Kept explicitly in the grounded operator and evaluated against the state at application time. |
-| `:derived-predicates` | 2.2 | **native** | `(:derived ...)` axioms. Rules are grounded and evaluated to a least fixpoint after every state change, so planners and heuristics always see closed states. Derived predicates may not appear in action effects. |
+| `:derived-predicates` | 2.2 | **native** | `(:derived ...)` axioms. Rules are grounded and re-evaluated from scratch to a least fixpoint after every state change, so planners and heuristics always see closed states and a derived fact whose support goes away is retracted. Negating a derived predicate in a precondition or goal is supported; negating one inside another rule, which needs stratification, is refused. Derived predicates may not appear in action effects. |
 | `:equality` | 1.2 | **native** | `=` between terms. Evaluated during grounding; infeasible instances are dropped. |
 | `:strips` | 1.2 | **native** | Add/delete effects over positive literals. The core representation. Everything else compiles down to this. |
 | `:typing` | 1.2 | **native** | Typed parameters and objects, with type hierarchies. Subtypes are resolved transitively when building the object pools. |

jupyddl/api.py

@@ -30,11 +30,22 @@ def solve_task(
     search as it runs. ``max_expansions`` and ``time_limit`` bound the run; when
     either is hit the planner stops and sets ``result.truncated``, so an empty
     result means "gave up", not "proved unsolvable".
+
+    A planner that needs a heuristic and is not given one gets ``lmcut`` if it
+    is optimal (so the default keeps the guarantee) and ``hff`` otherwise.
+    ``heuristic="none"`` asks for no guidance at all, which for a planner that
+    needs a heuristic means ``blind``.
     """
     planner = make_planner(search, **planner_kwargs)
     heur = None
-    name = heuristic if heuristic else ("hff" if planner.requires_heuristic else None)
-    if name is not None:
+    name = heuristic
+    if name == "none":
+        name = "blind" if planner.requires_heuristic else None
+    elif not name and planner.requires_heuristic:
+        # An optimal planner gets an admissible default: hff would quietly
+        # cost it its guarantee.
+        name = "lmcut" if planner.optimal else "hff"
+    if name:
         heur = make_heuristic(name, task)
     budget = make_budget(max_expansions, time_limit)
     return planner.search(task, heur, observer=observer, budget=budget)

jupyddl/cli.py

@@ -196,7 +196,7 @@ def _observers(args):
 
 def _cmd_solve(args) -> int:
     task = build_task(args.domain, args.problem)
-    heuristic = None if args.heuristic == "none" else args.heuristic
+    heuristic = args.heuristic  # "none" is resolved by solve_task
     kwargs = {"weight": args.weight} if args.search == "wastar" else {}
     observer, recorder = _observers(args)
     result = solve_task(
@@ -297,8 +297,7 @@ def _cmd_animate(args) -> int:
     from .viz import animate_search
 
     task = build_task(args.domain, args.problem)
-    heuristic = None if args.heuristic == "none" else args.heuristic
-    _, trace = trace_search(task, args.search, heuristic)
+    _, trace = trace_search(task, args.search, args.heuristic)
     animate_search(
         trace, args.output, dark=args.dark, fps=args.fps, seconds=args.seconds
     )

jupyddl/grounding.py

@@ -304,6 +304,13 @@ def _collect_effect(eff, subst, type_objects, cpos, cneg, acc):
         else:
             acc["delete"].add(atom)
     elif isinstance(eff, IncreaseCostEffect):
+        if cpos or cneg:
+            # An operator has one cost. Charging this unconditionally would
+            # bill for an effect that did not fire.
+            raise UnsupportedFeatureError(
+                "(increase (total-cost) ...) inside a 'when' is not supported: "
+                "an action's cost may not depend on the state"
+            )
         acc["cost"] += eff.amount
         acc["has_cost"] = True
     elif isinstance(eff, NumericEffect):
@@ -358,7 +365,23 @@ def walk(eff):
     return preds
 
 
+def _mentions_total_cost(expression) -> bool:
+    if isinstance(expression, FluentRef):
+        return expression.name == "total-cost"
+    if isinstance(expression, Arithmetic):
+        return _mentions_total_cost(expression.left) or _mentions_total_cost(
+            expression.right
+        )
+    return False
+
+
 def _ground_raw_operators(domain, problem, type_objects) -> list:
+    # Under a metric over (total-cost), an action that never increases it is
+    # free -- that is what the metric says, and what IPC planners do. Without
+    # one, every action costs 1 and cost-optimal means shortest.
+    action_costs = problem.metric is not None and _mentions_total_cost(
+        problem.metric[1]
+    )
     raw = []
     for action in domain.actions:
         pools = [type_objects.get(typ, ()) for (_, typ) in action.parameters]
@@ -385,11 +408,18 @@ def _ground_raw_operators(domain, problem, type_objects) -> list:
                 duration = _constant_value(action.duration, subst, action.name)
             if acc["has_cost"]:
                 cost = acc["cost"]
+            elif action_costs:
+                cost = 0
             elif duration:
                 # A temporal action with no explicit cost: optimise makespan.
                 cost = duration
             else:
                 cost = 1
+            if cost < 0:
+                raise PDDLError(
+                    f"action '{base}' has negative cost {cost}; "
+                    "(increase (total-cost) k) needs k >= 0"
+                )
 
             for index, disjunct in enumerate(disjuncts):
                 # Only tag the name when the split is real, so classical domains
@@ -527,6 +557,17 @@ def comp(self, atom: Atom) -> int:
         return self.comp_ids[atom]
 
 
+def _ids(ids) -> frozenset:
+    """A fact-id set built in sorted order.
+
+    A set of small ints iterates in insertion order wherever two ids share a
+    hash slot, so a set filled while walking atoms in hash order iterates
+    differently under each PYTHONHASHSEED -- enough to change which of two
+    tied facts a heuristic picks.
+    """
+    return frozenset(sorted(ids))
+
+
 def _ground_axioms(domain: Domain, type_objects: dict) -> list:
     """Ground every derived-predicate rule into (head atom, disjunct) pairs."""
     grounded = []
@@ -608,6 +649,31 @@ def resolve_literals(pos_atoms, neg_atoms):
                 cond.append((cpos_f, cneg_f, cadd, cdel))
         resolved.append((op, pre_pos_fluent, pre_neg_fluent, add, delete, cond))
 
+    # --- numeric fluents that are never defined ------------------------------
+    # A fluent with no value in :init that no action assigns stays undefined,
+    # and PDDL makes any condition or effect that reads it fail. Reading it as
+    # 0 instead would let `(distance a c)` for a road that does not exist cost
+    # nothing. (A fluent defined later by an effect is not tracked.)
+    assigned = {target for op, *_ in resolved for _, target, _ in op.numeric}
+    defined = set(problem.init_numeric) | assigned
+
+    def reads_undefined(comparisons, values=()) -> bool:
+        read: set = set()
+        for comparison in comparisons:
+            _collect_fluents(comparison.left, read)
+            _collect_fluents(comparison.right, read)
+        for value in values:
+            _collect_fluents(value, read)
+        return any(ref not in defined and ref.name != "total-cost" for ref in read)
+
+    resolved = [
+        entry
+        for entry in resolved
+        if not reads_undefined(
+            entry[0].comparisons, [value for _, _, value in entry[0].numeric]
+        )
+    ]
+
     # --- goal ----------------------------------------------------------------
     goal_disjuncts = _dnf(problem.goal, {}, type_objects)
     if not goal_disjuncts:
@@ -617,7 +683,7 @@ def resolve_literals(pos_atoms, neg_atoms):
     unsolvable = True
     for disjunct in goal_disjuncts:
         parts = resolve_literals(disjunct.pos, disjunct.neg)
-        if parts is None:
+        if parts is None or reads_undefined(disjunct.comparisons):
             continue  # this way of satisfying the goal is statically impossible
         unsolvable = False
         resolved_goals.append((parts[0], parts[1], disjunct.comparisons))
@@ -630,6 +696,12 @@ def resolve_literals(pos_atoms, neg_atoms):
         parts = resolve_literals(disjunct.pos, disjunct.neg)
         if parts is None:
             continue
+        if any(atom.predicate in derived_preds for atom in parts[1]):
+            raise UnsupportedFeatureError(
+                f"the rule for {head} negates a derived predicate; that needs "
+                "stratified axiom evaluation, which is not supported (negating "
+                "a derived predicate in a precondition or goal is fine)"
+            )
         resolved_axioms.append((head, parts[0], parts[1]))
 
     # --- numeric fluents -----------------------------------------------------
@@ -656,12 +728,31 @@ def resolve_literals(pos_atoms, neg_atoms):
     )
 
     # --- encode facts --------------------------------------------------------
+    # Fact ids are handed out in sorted order. Assigning them while walking
+    # sets of atoms ties every id to string hashing, so PYTHONHASHSEED changed
+    # LM-cut's tie-breaking and with it the expansions and the plan.
+    every_atom: set = {a for a in init_atoms if not is_static(a)}
+    for op, pp, pn, add, delete, cond in resolved:
+        every_atom |= pp | pn | add | delete
+        for cpos_f, cneg_f, cadd, cdel in cond:
+            every_atom |= cpos_f | cneg_f | cadd | cdel
+    for goal_pos, goal_neg, _ in resolved_goals:
+        every_atom |= goal_pos | goal_neg
+    for head, body_pos, body_neg in resolved_axioms:
+        every_atom |= {head} | body_pos | body_neg
+    for atom in sorted(every_atom, key=str):
+        enc.fact(atom)
+    for atom in sorted(tracked_neg, key=str):
+        enc.comp(atom)
+
     init_ids = {enc.fact(a) for a in init_atoms if not is_static(a)}
     for atom in tracked_neg:
         cid = enc.comp(atom)
         if atom not in init_atoms:
             init_ids.add(cid)
 
+    complement_ids = frozenset(enc.comp(a) for a in tracked_neg)
+
     def encode_add_del(add_atoms, del_atoms):
         add_ids = {enc.fact(a) for a in add_atoms}
         del_ids = {enc.fact(a) for a in del_atoms}
@@ -671,18 +762,23 @@ def encode_add_del(add_atoms, del_atoms):
         for a in del_atoms:
             if a in tracked_neg:
                 add_ids.add(enc.comp(a))
-        return frozenset(add_ids), frozenset(del_ids)
+        # An atom both added and deleted ends up true (add wins), so its
+        # complement must end up false: for complements the delete wins.
+        add_ids -= del_ids & complement_ids
+        return _ids(add_ids), _ids(del_ids)
 
     operators = []
     for op, pp, pn, add, delete, cond in resolved:
-        precond = {enc.fact(a) for a in pp} | {enc.comp(a) for a in pn}
+        precond = _ids({enc.fact(a) for a in pp} | {enc.comp(a) for a in pn})
         add_ids, del_ids = encode_add_del(add, delete)
         cond_effects = []
+        touched = set(add_ids)
         for cpos_f, cneg_f, cadd, cdel in cond:
             cond_ids = {enc.fact(a) for a in cpos_f} | {enc.comp(a) for a in cneg_f}
             cadd_ids, cdel_ids = encode_add_del(cadd, cdel)
             if cadd_ids or cdel_ids:
-                cond_effects.append(CondEffect(frozenset(cond_ids), cadd_ids, cdel_ids))
+                cond_effects.append(CondEffect(_ids(cond_ids), cadd_ids, cdel_ids))
+                touched |= cadd_ids
         numeric_pre = tuple(_compile_comparison(c, index_of) for c in op.comparisons)
         numeric_eff = tuple(
             _compile_numeric_effect(kind, index_of[target], value, index_of)
@@ -691,14 +787,17 @@ def encode_add_del(add_atoms, del_atoms):
         operators.append(
             Operator(
                 op.name,
-                frozenset(precond),
+                precond,
                 add_ids,
                 del_ids,
                 tuple(cond_effects),
                 op.cost,
                 numeric_pre,
                 numeric_eff,
                 op.duration,
+                complements=(
+                    frozenset(touched) & complement_ids if cond_effects else frozenset()
+                ),
             )
         )
 
@@ -719,7 +818,7 @@ def encode_add_del(add_atoms, del_atoms):
             operators.append(
                 Operator(
                     name,
-                    frozenset(precond),
+                    _ids(precond),
                     frozenset({goal_fact}),
                     frozenset(),
                     (),
@@ -738,9 +837,7 @@ def encode_add_del(add_atoms, del_atoms):
     axioms = tuple(
         Axiom(
             enc.fact(head),
-            frozenset(
-                {enc.fact(a) for a in body_pos} | {enc.comp(a) for a in body_neg}
-            ),
+            _ids({enc.fact(a) for a in body_pos} | {enc.comp(a) for a in body_neg}),
         )
         for head, body_pos, body_neg in resolved_axioms
     )
@@ -757,11 +854,21 @@ def encode_add_del(add_atoms, del_atoms):
         direction, expression = problem.metric
         metric = f"{direction} {expression}"
 
+    # Before Task(...): encoding a derived atom no rule produces adds a name,
+    # and `facts` below must already include it.
+    derived_complements = tuple(
+        sorted(
+            (enc.fact(atom), enc.comp(atom))
+            for atom in tracked_neg
+            if atom.predicate in derived_preds
+        )
+    )
+
     return Task(
         name=problem.name or domain.name,
         facts=tuple(enc.names),
-        init=frozenset(init_ids),
-        goals=frozenset(goals),
+        init=_ids(init_ids),
+        goals=_ids(goals),
         operators=tuple(operators),
         metric_cost=problem.metric_minimize_cost,
         axioms=axioms,
@@ -773,6 +880,7 @@ def encode_add_del(add_atoms, del_atoms):
         metric=metric,
         requirements=requirements,
         synthetic=frozenset(synthetic),
+        derived_complements=derived_complements,
     )