OK, I added this with a commit directly in the main branch.

Looks very good to me!

Should I add an entry like this?

- For developers: We cleaned up the code of LandmarkGraph. Some of the public
  methods were renamed. This class will undergo further changes in the future.
  http://issues.fast-downward.org/issue989

We do mention things that are relevant for developers only because we assume that a good part of our target audience wants to work on the code and not just use it as-is. But we are generally more brief with these and put them near the end of the changelog to emphasize the user-facing changes.

To see some examples, look at existing entries beginning with "For developers:".

I also don't know what our convention is for such cases. In my opinion, we wouldn't need an entry.

By the way, I assumed that we don't want a change log message for this because it is a cosmetic change that is not relevant for users. Is that correct, or do we still want an entry?

Great, thanks. The tests ran through and I merged the issue.

I can confirm that the dump method works.

Good that you also include the fix to the local test, I had that in issue990 as well (but didn't include the fix as I wasn't sure about the convention regarding non-issue related changes like this)

I just merged with issue990 and pushed to the issue branch. 
https://github.com/aibasel/downward/pull/21
Do you want to test if the dump method works in your context before we merge this?

Otherwise I would merge this soon. My local test showed an error in the LP solver code that didn't show on the github actions (I guess due to an issue with the clang-tidy version). I fixed it (https://github.com/aibasel/downward/pull/21/commits/f8e3b07d) but now the Github actions started from scratch, so I have to wait another iteration before the merge.

Neither do I understand why it crashed, but passing a TaskProxy to dump did it for me. I can only assume that the TaskProxy object inside the LandmarkGraph class somehow gets corrupted, but then I don't know enough about the proxy objects. Thanks for checking!

Issue990 has been merged yesterday, so I believe this issue is next on the list. If you need help merging because of merge conflicts with issue990 let me know!

I moved the method out of the class, so now you have to pass in the correct task proxy. I don't understand why it crashed before, so I don't know if this fixes it but if your fix work, then it does sound like merging this issue would fix the crash.

Currently, the LandmarkGraph::dump() method makes the planner crash, presumably because of the TaskProxy object (I managed to get rid of the crash by passing a new TaskProxy object to dump). Clemens said that this might have been resolved in this issue. Florian, can you verify this?

We discussed the remaining issues live. I will merge and push once issue990 is merged.

I did one more change after our discussion flagged by the github actions: the task_proxy was no longer used in the class, so I removed it.

I had a look at the changes of LandmarkGraph and left a few comments.

I don't think experiments are needed. Regarding coordinating, it could be best to discuss this on discord/in a break today.

I left one review comment. There are a few changes that affect performance (changes in how vectors are constructed), but if you think no experiments are needed, I'm fine with that. Merging this should probably be coordinated with merging the other landmark issues that are ready to be merged (or close to being ready).

I would suggest a new issue rather than issue334. It's good to have more specific issues rather than ones with catch-all names. And this is a change that I would not combine with other changes because it requires its own experiments etc.

For this issue, I'd suggest to keep things to only renaming. I'm fine with either cost or min_cost.

Addressing the issue of not having costs in the landmark factories should happen somewhere else. Maybe we could use issue334 for this, which I would otherwise consider to close because being done/addressed by newer issues linked from issue987.

Yes, and this difference in behaviour between different landmark factories isn't the intended one.

From the intention, landmark factories should find landmarks and landmark orderings, and nothing more. The cost of a landmark isn't an intrinsic property but a derived feature that should be computed by the heuristic based on the achievers (and possibly first achievers). Because there wasn't originally a clean separation between factories and heuristics, these costs ended up on the factory side and now we see that they are handled inconsistently.

Presumably when action costs were added to Fast Downward (or rather LAMA at the time, which was then a fork), lm_rhw was the only generation method that was updated to take action costs into account because it was the only generation method anyone cared about.

Maybe I'm explaining it wrong. The constructor has a default "cost" of 1 and most landmark factories leave it at that (lmcount then adds these "costs" to effectively count the landmarks). One factory uses a the minimal cost of any achiever in place of this default cost of 1, so lmcount adds up these costs instead of counting the landmarks.

> Thanks for the review. I thought about "min_cost" for a while as well. While the
> other factories do not set any values here, they rely on the default value of 1. In
> the heuristic this is then used as the contribution of this landmark to the
> heuristic value.

This just sounds like a bug that should be fixed.

Without having looked at the code and having to guess a little bit at the contexte, "cost" sounds good to me because this is what the literature uses.

No matter what the name is, I don't think the landmark factories have any business setting costs here, and there is certainly no reason for LM_RHW to be different from the others. In other words, this probably needs to be changed. (Unless I'm misunderstanding what this is about.)

Thanks for the review. I thought about "min_cost" for a while as well. While the other factories do not set any values here, they rely on the default value of 1. In the heuristic this is then used as the contribution of this landmark to the heuristic value. I thought the "cost of the landmark" more or less covers this idea but I'm fine with other names as well. I just thought min_cost is not appropriate (especially with the old documentation of "minimal cost of achieving operators", which is just wrong for other factories).

I have had a look at your changes. There's not much to say in my opinion. The only thing I'm not so sure about is the renaming of the min_cost member to cost. Both names do not seem appropriate as only the LandmarkFactoryRpgSasp sets this value, otherwise it holds its initial value (which is 1). Hence, for all other factories it has nothing to do with a cost at all, but for the RpgSasp (= RHW landmarks) case, it really is the min_cost.

Other than that, there is an outdated call to dump the landmark graph, which is commented out anyways. Everything else looks good to me.

I prepared a pull request 
https://github.com/aibasel/downward/pull/21

It might be easier to review this by looking at individual commits (I tried keeping each of them small and simple):

https://github.com/aibasel/downward/pull/21/commits

Can someone have a look?

When working on issue988, we discussed that the code readability could be improved by changes like renaming variables or moving implementations to the source file instead of the header (i.e., the LandmarkNode class is implemented entirely in landmark_graph.h).