Issue999

[thomaskeller79]

Pull request for issue999.

[FlorianPommerening]

I looked at most the diff. I didn't get around to two parts, but I'm running out of time now. I marked them, so I can have a look at them in the next iteration.

[FlorianPommerening]

I don't understand why there are two parameters disjunctive and conjunctive. What happens if I set both to true? What happens if I use multiple facts and set none of them to true?

[FlorianPommerening]

Having so many boolean parameters also makes the the code harder to read (especially with default values on the parameters). That is, without looking up the constructor, I don't know what Landmark lm(facts, true, false, true, false) will do and how it is different from Landmark lm(facts, true, false). I have some suggestions how to reduce this:

• Instead of disjunctive and conjunctive, I'd add a enum class LandmarkType with the values disjunctive, conjunctive, and atomic.
• If possible, I'd compute is_true_in_goal inside the constructor instead of passing it as a parameter.
• I don't know what is_derived does but I'd consider making it a mandatory argument.

[thomaskeller79]

I totally agree with this comment (and many others below), but we intentionally chose not to alter the code on this level and opt for a smaller diff instead as the landmark code is full with issues like this one. As this issue touches almost every landmark file, addressing issues on this level would presumably mean that we can as well re-write the whole landmark code. Instead, I'd suggest to address issues with the code like this one with separate "refactor class X" issues at some point in the future.

[thomaskeller79]

I'm going to mark all comments below where I want to refer to this comment with "out of scope of issue". If you agree, please resolve those comments.

[ClemensBuechner]

I don't understand why there are two parameters disjunctive and conjunctive. What happens if I set both to true? What happens if I use multiple facts and set none of them to true?

We decided to cover this using assertions for now. It will be the goal of issue1023 to make this distinction between conjunctive, disjunctive, and atomic landmarks more explicit by introducing specific classes that are derived from the Landmark class (opposed to introducing an enum here).

* If possible, I'd compute `is_true_in_goal` inside the constructor instead of passing it as a parameter.

As far as I know, is_true_in_goal is something that doesn't need to be computed in most landmark factories but is already known when the landmark is inserted into the landmark graph. Hence, I wouldn't change this, at least not in this issue.

[FlorianPommerening]

I was just catching up on what happened in the sprint. I'm of course fine with deferring the comments to issue1023. On the point about is_true_in_goal, we should have a look at if this really saves some effort. I think the code would be much cleaner if this was not a parameter so I would even accept a small hit to performance in this case (but I doubt recomputing this would have a measurable impact).

[thomaskeller79]

I made a note in issue1023 that this comment is still open. I suggest to implement the suggested change there (regardless of the changes to disjunctive and conjunctive I agree this would make the code cleaner) and evaluate how it affects performance.