This is akin to --keep-unimportant-variables, but for the operaters/actions.

We'd like the command line to have a translator option to --keep-noop-operators 
so that they 
aren't discarded. This touches on several places where an operator or action 
(depending on where 
in the process you are) is removed if the effects are empty. Default should be 
false, but this 
would help those of us using the FD translator for other paradigms (FOND, PPOS, 
PPDDL, etc).

Merged (yesterday). Thanks a lot, Christian!

Looks ready to merge to me. I'll give everyone a bit of time to react before doing this. I've set a reminder for Thursday, but not sure if I'll get to it (currently travelling).

Experiment done.

Notes to reproduce:

Tags created manually on grid as follows:
$ git show-ref   --tags
89b3d973cdd1559f239302a446d3aba702a6af49 refs/tags/issue1184-base
5e0f533dcc815570930e0b297c15be1fc38247cc refs/tags/issue1184-v1

Compare if translator output is the same as follows:
$ ./compare_hashes.py data/*/properties


I ran two experiments.

The first experiment compared the previous code (base) to the new code (v1) without the new flag enabled. This is primarily a sanity test:

https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1184/data/issue1184-v1_vs_base-eval/issue1184-v1_vs_base.html

With both configs, the translator failed on 128 tasks and succeeded on 3960. It failed on the same set of tasks, and when it succeeded, the translator output file was always the same (by comparing hashes for output.sas).

The run showed a bunch of "unexplained errors" which are either our parser being too strict or the benchmarks containing PDDL bugs. I think these are on domain formulations we don't usually exercise. It would be nice to do something about it, but it's not related to this issue and I'll ignore it.

Otherwise I see nothing worrisome in the report.


The second experiment compared the new code without the flag enabled to the new code with the flag enabled:

https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1184/data/issue1184-v1_with_and_without-eval/issue1184-v1_with_and_without.html

With the flag enabled, we see a difference in the output in 1208 benchmarks, so presumably this is the number of benchmarks where the default config prunes actions without effects. I didn't try to systematically check this.

There are 5 additional tasks we fail to translate when we include noops.

Looking at per-domain number of operators (translator_operators) and runtime (translator_done), the numbers seem to make sense. Looks like in some caldera formulations, more than half of the operators are no-ops. But if this is an ADL domain, this may have a lot to do with the reformulations we do along the way.

These are the number of affected benchmarks by domain:

      1  airport
      1  assembly
     20  caldera-opt18-adl
     16  caldera-sat18-adl
     20  caldera-split-opt18-adl
     20  caldera-split-sat18-adl
     20  childsnack-opt14-strips
     20  childsnack-sat14-strips
     20  data-network-opt18-strips
     20  data-network-sat18-strips
     22  depot
      6  driverlog
     20  floortile-opt11-strips
     20  floortile-opt14-strips
     20  floortile-sat11-strips
     20  floortile-sat14-strips
     20  gripper
     20  hiking-agl14-strips
     20  hiking-opt14-strips
     20  hiking-sat14-strips
      5  labyrinth-opt23-adl
     28  logistics00
     35  logistics98
      5  maintenance-opt14-adl
    145  miconic-fulladl
    145  miconic-simpleadl
     17  organic-synthesis-split-opt18-strips
     17  organic-synthesis-split-sat18-strips
     30  pathways
     39  psr-large
     30  psr-middle
     44  psr-small
     20  quantum-layout-opt23-strips
     20  quantum-layout-sat23-strips
     40  rovers
     36  satellite
      9  scanalyzer-08-strips
      5  scanalyzer-opt11-strips
      6  scanalyzer-sat11-strips
     20  spider-opt18-strips
     20  spider-sat18-strips
     20  thoughtful-mco14-strips
     20  thoughtful-sat14-strips
     30  woodworking-opt08-strips
     20  woodworking-opt11-strips
     30  woodworking-sat08-strips
     20  woodworking-sat11-strips
      6  zenotravel


For anyone interested in the experiment code, raw properties file and more:

https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1184/

Thanks for having a look into it! I'd feel more comfortable having the full test 
to check for regressions with the code & corner cases, so no worries.

Thanks, looks good! I left one last polishing comment.

For me it looks ready to merge, but I would still like to run an experiment first, which is taking me a bit of time to set up because I haven't done it in a while and there have been some significant changes in our infrastructure. I hope I can finish the setup and queue the experiment later today.

Thanks! Changes make sense -- revisions are now all set. I'm attaching a slightly 
changed gripper domain (you can pair it with 
misc/tests/benchmarks/gripper/prob01.pddl) that has a noop operator for testing.

Hi Christian, thanks for the pull request and sorry for the slow reaction.
I'm now off the busy stretch and expect my next reaction should come much more quickly.

I'd like to request a change in the style of the code -- rather than passing the new option around a lot, access it from the options module where needed. That, plus one or two smaller things. See my review of the pull request for more details and some rationale. This should also make the pull request much smaller and touch fewer files.

Let me know if you are OK to make these changes. Otherwise I can also try to make them myself.

Can you attach or link one or two benchmarks that trigger the new changes, so that we can test this?

I don't think it's very important for this specific change, but as a matter of best practice I'd also like to run a larger before-after translator experiment before we merge. I'll see if I can set something up.

pull request: https://github.com/aibasel/downward/pull/263