TODOs:

- Once this is resolved, report back to Emil, e.g. in the Fast Downward mailing
list thread, replying to Malte's email that referenced this issue (21.05.2019
16:54 CEST).

Thanks Remo for the review! The change has been merged.

I fixed the bug as discussed. Any takers for a review?

We see the effect of the change clearly in the output of the model computation (translator_auxiliary_atoms, translator_relevant_atoms, translator_final_queue_length, translator_queue_pushes). Auxiliary atoms are the ones included when splitting rules of the Prolog program. The atoms added for taking care of the numeric expressions are counted towards the relevant atoms. I am somewhat surprised that the number of auxiliary atoms and queue_pushes decreases in some domains. Malte, you know the inner workings of the Prolog model transformations. Can this make sense?

Apart from this, the runtime for generating and normalizing the program is only mildly affected. Computing the model is slightly slower in some domains but the changes in the overall time for instantiation looks ok to me (overall a few seconds faster but lots of variance in both directions).

Since the bug did not occur on the benchmarks, the instantiated operators are not affected (as expected).

Pull request: https://github.com/aibasel/downward/pull/216
Experiments: https://ai.dmi.unibas.ch/_experiments/ai/downward/issue913/data/issue913-v1-eval/issue913-v1-23499409976f669b036ef162b2835832f01d2fb2-issue913-v1-compare.html

We agreed to resolve this in the reachability analysis.

I think the warnings would definitely be useful, though. Perhaps we can make
them an optional feature.

Making this part of the grounding algorithm would have the advantage that we
could also eliminate other, then unreachable action.

My first impulse was to integrate the testing of well-definedness in the
grounding algorithm (for efficiency), but then we cannot easily get such
warnings because the failing actions are never generated.

This was mentioned by Emil Keyder on the mailing list.

If some action refers to an uninitialized numeric expression, the translator
stops with "AssertionError: Could not find instantiation for PNE!". The correct
behaviour would be to simply not instantiate this action.

Maybe we should in this case include an output that the action was removed,
because otherwise this behaviour could easily hide modelling mistakes.