Created on 2025-05-12.15:52:37 by haz, last changed by malte.
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).
|
File name |
Uploaded |
Type |
Edit |
Remove |
domain.pddl
|
haz,
2025-06-24.18:04:46
|
application/octet-stream |
|
|
msg11857 (view) |
Author: malte |
Date: 2025-07-18.14:14:08 |
|
Merged (yesterday). Thanks a lot, Christian!
|
msg11855 (view) |
Author: malte |
Date: 2025-07-15.18:11:02 |
|
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).
|
msg11854 (view) |
Author: malte |
Date: 2025-07-15.18:10:08 |
|
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/
|
msg11850 (view) |
Author: haz |
Date: 2025-06-25.15:21:46 |
|
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.
|
msg11849 (view) |
Author: malte |
Date: 2025-06-25.12:02:42 |
|
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.
|
msg11848 (view) |
Author: haz |
Date: 2025-06-24.18:04:46 |
|
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.
|
msg11847 (view) |
Author: malte |
Date: 2025-06-24.14:59:42 |
|
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.
|
msg11846 (view) |
Author: malte |
Date: 2025-06-24.14:27:51 |
|
pull request: https://github.com/aibasel/downward/pull/263
|
|
Date |
User |
Action |
Args |
2025-07-18 14:14:08 | malte | set | messages:
+ msg11857 |
2025-07-15 18:11:02 | malte | set | messages:
+ msg11855 |
2025-07-15 18:10:08 | malte | set | messages:
+ msg11854 |
2025-06-25 15:21:46 | haz | set | messages:
+ msg11850 |
2025-06-25 12:02:42 | malte | set | messages:
+ msg11849 |
2025-06-24 18:04:46 | haz | set | files:
+ domain.pddl messages:
+ msg11848 summary: 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). -> 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). |
2025-06-24 14:59:42 | malte | set | messages:
+ msg11847 |
2025-06-24 14:27:51 | malte | set | status: unread -> chatting assignedto: malte messages:
+ msg11846 |
2025-05-12 18:39:14 | malte | set | nosy:
+ malte, haz, jendrik |
2025-05-12 15:52:37 | haz | create | |
|