Title Remove dead code from landmark exploration.
Priority wish Status resolved
Superseder Nosy List clemens, jendrik, malte, remo, salome, silvan, thomas
Assigned To Keywords
Optional summary

Created on 2022-02-08.17:08:04 by clemens, last changed by clemens.

msg10630 (view) Author: malte Date: 2022-03-01.14:22:29
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.
msg10629 (view) Author: clemens Date: 2022-03-01.13:40:03
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.
msg10628 (view) Author: malte Date: 2022-03-01.12:00:07
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.
msg10627 (view) Author: clemens Date: 2022-02-28.16:06:02
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:
msg10626 (view) Author: malte Date: 2022-02-28.15:57:17
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.
msg10625 (view) Author: clemens Date: 2022-02-28.09:51:04
Here are the new experiment results:

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.
msg10624 (view) Author: clemens Date: 2022-02-25.17:37:43
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. ;-)
msg10623 (view) Author: malte Date: 2022-02-25.17:25:49
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.")
msg10622 (view) Author: clemens Date: 2022-02-25.17:21:40
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...
msg10621 (view) Author: malte Date: 2022-02-25.17:14:12
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.
msg10620 (view) Author: malte Date: 2022-02-24.14:01:30
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.
msg10619 (view) Author: malte Date: 2022-02-24.13:33:42
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.
msg10618 (view) Author: clemens Date: 2022-02-24.10:34:27
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:

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.

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?
msg10546 (view) Author: malte Date: 2022-02-08.18:12:06
We also noticed that *depth* in Exploration is never used (except to compute other values of *depth* ;-)).
msg10540 (view) Author: clemens Date: 2022-02-08.17:08:04
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.
Date User Action Args
2022-03-01 20:21:47clemenssetstatus: chatting -> resolved
2022-03-01 14:22:29maltesetmessages: + msg10630
2022-03-01 13:40:03clemenssetmessages: + msg10629
2022-03-01 12:00:07maltesetmessages: + msg10628
2022-02-28 16:06:02clemenssetmessages: + msg10627
2022-02-28 15:57:17maltesetmessages: + msg10626
2022-02-28 09:51:04clemenssetmessages: + msg10625
2022-02-25 17:37:43clemenssetmessages: + msg10624
2022-02-25 17:25:49maltesetmessages: + msg10623
2022-02-25 17:21:40clemenssetmessages: + msg10622
2022-02-25 17:14:12maltesetmessages: + msg10621
2022-02-24 14:01:30maltesetmessages: + msg10620
2022-02-24 13:33:42maltesetmessages: + msg10619
2022-02-24 10:34:27clemenssetmessages: + msg10618
2022-02-09 09:43:21silvansetnosy: + silvan
2022-02-08 18:12:06maltesetmessages: + msg10546
2022-02-08 17:33:46thomassetnosy: + thomas
2022-02-08 17:08:04clemenscreate