[issue1187] Failing Assertion in Enforced Hill Climbing on Trivial Problem

[speckdavid]

Fix: assert(num_ehc_phases != 0); => assert(num_ehc_phases >= 0); to handle trivial PDDL instances where the initial state already satisfies the goal.

[maltehelmert]

I think we should think about why the assertion is here. Why do you want to assert that it's nonnegative? Why is the value 0 ok here but a negative value is not?

The next statement gives a hit: if num_ehc_phases is 0, we will divide by zero. I assume you tested it and the code worked with the new assertion regardless. But that is not guaranteed; floating point division by zero in C++ is undefined, and one possible behaviour is to raise a hardware exception and terminate the program.

So I think we need to make a different change. I'm not a big fan of conditional logging (i.e., only printing the line if the number of phases is 0) because it can lead to unintended behaviour in automated benchmarks. Perhaps we should question whether logging such an average is a good idea in the first place. To judge this, I guess it would be good to see the output in the context of what else is logged. Should we discuss this in person briefly?

[speckdavid]

Good catch! (And yes, it worked regardless, but we should still fix this.)

Sure, let's discuss it in person when you have time.

[speckdavid]

I checked Lab and it doesn't include any EHC-specific parsing in the included parsers.

So, as discussed, I removed the logging of "Average expansions per EHC phase:" and the related assertion.

(I’ve summarized our offline discussion on the issue tracker.)