This is part of meta issue987.

We (Gabi, Florian, Simon and I) have looked at the code diff and experiments as part of the ongoing sprint. We think the idea of this issue was to make the code clearer by splitting the logic of the landmark class into the different variants of landmarks we use (disjunctive, conjunctive or simple). Further, true type would be hidden wherever it is irrelevant. Looking at the suggested implementation revealed that there are a lot of places where we actually need to know which type of landmark we're dealing with, and that this distincition is not easily movable into the classes implementing the different types. So we don't think that this is a reasonable approach to make the code clearer anymore. Thus, we close this issue without integrating any changes.

Looking at 
https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1023/data/scripts-v1-optimal-exp2-issue1023-base-issue1023-v1-compare.html
and
https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1023/data/scripts-v1-satisficing-exp2-issue1023-base-issue1023-v1-compare.html

With the optimal-suite the (search/total) time scores go down by ~1.7 for configuration
	['--search', 'let(lmc, lmcount(lm_exhaust(), admissible=true), astar(lmc,lazy_evaluator=lmc))']
for basically all domains, at most for sokoban-opt08-strips and sokoban-opt08-strips ~0.13 each
In these two domains the score declines for most of the tasks (or stays the same) with the most extreme decline in sokoban-opt08-
strips/p22.pddl of 0.0127


with the satisficing-suite the search time scores go down on all configurations by at least ~0.5. For configuration
	['--search', 'eager_greedy([lmcount(lm_zg())])']
the decline is the most extreme with 2.12.
For this configuration almost all domains decline in the search time score. Most extreme are Schedule (0.27) and visitall-sat14-strips 
(0.36)

In those two domains the decline is distributed rather evenly over the individual tasks.

The search time score declines for visitall-sat14-strips with all tested configurations (or stays 0 in the hm2 cases)


In the hm2 cases: (the difference is the pref flag for the lmcount)
	['--evaluator', 'hlm=lmcount(lm_factory=lm_reasonable_orders_hps(lm_hm(m=2)),transform=adapt_costs(one),pref=true)', '--
evaluator', 'hff=ff(transform=adapt_costs(one))', '--search', 'lazy_greedy([hff,hlm],preferred=[hff,hlm], 
cost_type=one,reopen_closed=false)']
	and
	['--search', 'let(hlm, lmcount(lm_factory=lm_reasonable_orders_hps(lm_hm(m=2)),transform=adapt_costs(one),pref=false),let(hff, 
ff(transform=adapt_costs(one)),lazy_greedy([hff,hlm],preferred=[hff,hlm],\n cost_type=one,reopen_closed=false)))']
	
does not decline as extreme in the search time score because it has some strong improvement in freecell (~3) but for other domains 
there is similar overall decline most extreme in the woodworking-sat{08,11}-strips and barman-sat{11,14}-strips


I know we expected ~some~ decline but I do not know how much was expected nor how much is acceptable.

I do not know how to proceed from here.

The unexpected changes could come from nondeterministic behavior of the translator

The translator has been non-deterministic in both citycar and trucks in the past. Could that be the cause of the unexpected changes?

There is a first pull request for a Landmark base class and some experiments. 
We expected some decline in performance which the results confirm.

However, we did not expect changes in the "orderings - Sum" nor in the 
"landmarks - Sum". 
The orderings are most prominent in 
https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1023/data/scripts-v1-satisficing-exp2-issue1023-base-issue1023-v1-compare.html#orderings-trucks-strips

and 

the landmarks differ only in 
https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1023/data/scripts-v1-satisficing-exp2-issue1023-base-issue1023-v1-compare.html#landmarks-citycar-sat14-adl

The next step is to dig deeper into the unexpected behavior of citycar-sat14-adl.


PR: https://github.com/aibasel/downward/pull/150
experiment data: https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1023/

This has been created initially as issue999, where we split off the part of the issue that deals with splitting the LandmarkNode class into a Landmark and LandmarkNode class. The original issue of creating different types of landmarks (disjunctive, conjunctive and simple) that inherit from a common base class is now part of this issue.