Related to issue467 and issue937.

And it is merged :)

Great, looks good to merge! :-)

Oh dear, I even prepared a changelog and stashed it because I wanted to wait committing it until the http/https problem was fixed... I commited it now.

Before we can merge it, it needs a change log entry.

I have nothing to add, looks good to merge from my perspective.

After issue1041 was merged, I ran the experiment:
https://ai.dmi.unibas.ch/_tmp_files/simon/issue998-v1-satisficing-issue998-base-issue998-v1-compare.html#summary

It reports one segfault more, but that is a caldera task where base ran out of memory, so I assume base would have segfaulted as well. Otherwise we have 4 landmarks more since the 4 problems that previously caused exitcode248 now have 1 landmark each instead of an empty graph. 

Finally, landmark generation time went down a bit but I assume this is random noise. On the four tasks where the generation actually does something different now there is no measurable difference (which is not surprising, we still basically do the same thing but just additionally create one landmark).

For me this looks good to merge, what do others think?

Testing only this configuration sounds good. If I understand correctly, this will address the crash by making sure there is a landmark (the unreachable goal fact)?

Regarding SegmentedArrayVector, at minimum we should add an assertion and mention in the code comments that length 0 is not supported.

Oups sorry, I fixed the comment style.

Unless you think more than one configuration needs to be tested, I will start an experiment with "lmcount(lm_zg(reasonable_orders=false))" (this is the only configuration from the summary in issue987 that uses zg).

I looked a bit into the 0-length problem; concretely the code crashes when we try to divide by 0 in the constructor of SegmentedArrayVector (when trying to figure out how many arrays fit in a segment). I don't have a concrete idea how to support it. I'm also ok with leaving it as it is and only deal with it if it becomes relevant again.

I think we shouldn't merge code that we didn't test, so I think some experiments are always necessary. They don't have to be extensive.

The pull request doesn't follow our conventions for comments, we don't start every line in a multi-line comment with "*". I don't think the automated style checks catch this.

Regarding the 0-length per-state bitsets/arrays, if we want to address this, we should create an issue for it. But I would be slightly against opening an issue without also dealing with it immediately -- it looks like the kind of thing where nobody will deal with it if we just open an issue and let it sit there.

As discussed in issue1011, when a task is relaxed unsolvable we want lm_zg to return a landmark graph containing (at least) one landmark which represents an unreachable goal fact. I implemented this and created a pull request:
https://github.com/aibasel/downward/pull/86

It's a very local change and I don't think we need to discuss this much. It could of course be done differently. For example we could simply let the graph be built as normal and abort as soon as we encounter a goal fact that is relaxed unreachable, but I decided against it since it could waste a lot of time (for example if only the last goal fact considered is unreachable) and the check in the beginning is fast. Regarding experiments I'm not sure if its worth running some.

This does however not fix the issue with PerStateBitset not being able to deal with vectors of size 0. While it should not actually happen at the moment, I think adding a guard or or making it supported would still be a good idea.

> A simple solution would be to exit_with(UNSOLVABLE) if the factory detects the
> task as unsolvable.

This is not really a solution. We don't know in which context the landmark graph is generated. It's easy to imagine that it's applied to some transformed task that is unsolvable, while the planning task solved at the top level is solvable (and solved by some other heuristic, for example in an alternation open list).

> Since there are no landmarks, the landmarks_reached PerStateBitset has vector
> size 0 and when we try to retrieve the Bitset for a specific state it results
> in a division by 0.

This sounds like an error in itself. Not finding any landmarks should be legitimate. Or at least if we don't want to allow it, we should make that clear, which we don't currently do.

> Furthermore, we should add a guard for creating PerStateBitsets (and possibly
> more general PerStateInformation) where the length of the information is 0.

Either that or make it supported. I think "PerStateInformation" has no notion of length, but there is another class that does ("PerStateArray"?).

If we use the lm_zg landmark factory on a task which it detects unsolvable, it simply adds 0 landmarks to the graph and returns normally. Aside from not making sense on a conceptual level, this causes a floating point error down the line: Since there are no landmarks, the landmarks_reached PerStateBitset has vector size 0 and when we try to retrieve the Bitset for a specific state it results in a division by 0.

A simple solution would be to exit_with(UNSOLVABLE) if the factory detects the task as unsolvable. It would however in my opinion also be good to see how other landmark factories handle this issue (if they can detect unsolvable tasks at all) and have a unified approach. One option might be to have a flag in the landmark graph that indicates that the task is unsolvable; or to add only one (goal) landmark with a natural order on itself. For the latter, we would need to fix issue937 first.

Furthermore, we should add a guard for creating PerStateBitsets (and possibly more general PerStateInformation) where the length of the information is 0.