Happy to see this merged, thanks a lot! I just sent an email regarding the formulation of the change log entry, but it's not critical.

The remaining comments are addressed in the commit I pushed just now. I will merge as soon as you confirm that you're fine with these final changes.

Thanks! Experimental results look good. Perhaps they are slightly slower than v1, but nothing to worry about.

I think there are still some open pull request comments? For example, the "proposition"/"prop" thing.

Another faux-pas on my behalf, I'm really sorry. I did not overwrite the old files, I just posted the wrong links. Here are the corrected links with v2 instead of v1:

https://ai.dmi.unibas.ch/_tmp_files/buechner/issue1044-v2-optimal-compare.html
https://ai.dmi.unibas.ch/_tmp_files/buechner/issue1044-v2-satisficing-compare.html

These are the same URLs as before, did you overwrite the old results, and did this run the newest revision or the same as in the previous experiment? Normally we'd call this v2, keep v1 as the tag for the previous revision, and use v2 in the URLs instead of v1.

Here are the new experiment results:

https://ai.dmi.unibas.ch/_tmp_files/buechner/issue1044-v1-optimal-compare.html
https://ai.dmi.unibas.ch/_tmp_files/buechner/issue1044-v1-satisficing-compare.html

To me, they look similar to before, maybe a little bit worse concerning the landmark generation time in the satisficing configurations. Still, it's an overall improvement in this regard. Hence, I again suggest to merge at this point (after writing the changelog of course), assuming that we'll keep working on this code soon when addressing issue1045.

About the changelog entry: that is exactly the reason why I didn't commit it so far, but it's all prepared for the moment the experiment results arrive. ;-)

No worries. Normally I wouldn't have seen it, but in this case I reacted to the individual commit notifications to focus on the changes at the commit level rather than looking at the whole pull request again.

We still need the change log entry, but perhaps it's easiest to write this after having seen the experiments. Then we can perhaps also add some quantitative information about the effect. ("In our experiments, landmark generation time improves by ~X-Y% on average.")

Thanks for the reviw, I've addressed your comments and am running another set of experiments just to make sure we're still happy with the effect on planner performance.

About the "exploration_new" files: this was a stupid accident and I hoped to be fast enough with the force-pushing them away before anybody noticed. ;-) They were just copies of my local changes because I wanted to apply multiple changes I prepared in the exploration files in two separate commits, so I backed up the files and accidentally added them to the commit, sorry for the inconvenience...

Clemens applied some further code changes in reaction to the code review.

@Clemens: I've made some more comments, some of them directly as part of the pull request, some of them (somewhat accidentally) directly at individual commits. I hope both kinds arrived. I saw a commit that added two new source files "exploration_new.{cc,h}"; not sure if that was intentional. For some reason, I don't see them as part of the pull request even though I didn't see a commit that removed the files again.

Code looks good apart from the comments I made, which IIRC only affected code comments and variable names.

The experimental results look good. I would say the improvement in landmark generation time is very significant, the time is cut down by 10-20% on average depending on the exact configuration. I'm not surprised, we removed a lot of unnecessary computations from the critical part of the code.

I agree that's a good strategy. I left a code review for the pull request. Some comments don't need to be addressed in this issue, some are requests for simple changes (e.g. comments). Only one of them I think needs some discussion, related to the initialization of h_add values/exclusions for operators.

This also needs a change log entry.

I'll look at the experiments next.

Salomé and I started working on this issue yesterday. We implemented the obvious changes suggested in the previous messages (i.e., removed unused parts of code). Additionally, we replaced the previous integer *h_add_cost* with a boolean *excluded*. This is, because the *h_add_cost* was previously only used to denote whether a given proposition or operator should be excluded (plus the *h_add_cost* was updated if not excluded, but never used in this case).

Here is the link to the pull request: https://github.com/aibasel/downward/pull/97

We ran an experiment to see how these changes impact the planner performance. Since we mainly removed code and replaced an int with a bool, we expected it to reduce memory and have not much influence otherwise. As we can see in the experiment reports, this is indeed the case. Another observation is that landmark graph generation time is reduced (although I'm not sure whether this is significant) which makes sense because the code of the *landmarks::Exploration* class became a lot simpler. In the satisficing case, this even leads to increased total time scores by roughly 3 to 4 points.

https://ai.dmi.unibas.ch/_tmp_files/buechner/issue1044-v1-optimal-compare.html
https://ai.dmi.unibas.ch/_tmp_files/buechner/issue1044-v1-satisficing-compare.html

Now the question is how to procede from here. We have addressed the things that caught our eyes when working on issue1041. Since it is already quite some diff and we have positive experiment results, this might be a good time to merge this right away. However, it is possible that we have overlooked some dead code and might find more by investigating some more before resolving this issue. What do you think?

We also noticed that *depth* in Exploration is never used (except to compute other values of *depth* ;-)).

While working on issue1041, we discovered multiple pieces of dead code within *landmarks::Exploration* that should be removed.

- *level_out* in *Exploration::compute_reachability_with_excludes()* is always true,
- *use_h_max* in *Exploration::setup_exploration_queue()* is always true, and
- *compute_lvl_ops* in *Exploration::compute_reachability_with_excludes()* is always false (and would invoke buggy code if it was ever true).

There might be more that we didn't notice during our code walk.