feat: add new move count metric

[zepfred]

This pull request includes a new metric for counting the number of moves, which differs from the score count when using R&R moves.

[triceo]

I'm leaving a review before I leave for PTO.
It's OK if merged once my comments are resolved, provided that someone else sees the final PR as well.

[zepfred]

Optionally, the metric also has sub-metrics per move type. (Some other metrics can already do that, for inspiration.)

I couldn't come up with a useful sub-metric for the move count metric. Do you have any suggestions?

Naming. Is "move count" and "move speed" the right name?

For the benchmark report, it makes sense to me to present it as "Move Calculation Speed".

On the other hand, when defining the termination configuration, I think using only count is better: moveCountLimit and unimprovedMoveCountLimit. We don't have a termination configuration such as unimprovedScoreCountLimit, but I thought it might be useful to terminate after a certain number of unsuccessful moves are executed.

[zepfred]

@triceo, I see some possible points of discussion beside the ones you may find during the review:

Optionally, the metric also has sub-metrics per move type. (Some other metrics can already do that, for inspiration.)

I couldn't find any useful sub-metric for the move count. We already have the move count per step.

Solver logging additionally includes move speed (move count over time), along with score speed which it already does.

I initially included it but then removed it. My point is that it may confuse the user as it is the same number when not using R&R moves. I believe that the benchmark should be used to compare move speeds, and the score calculation speed should still be the primary metric.

There is a score count related type of Termination. We need one of those for move count as well.

I considered adding unimprovedMoveCount, but I'm unsure if it's worth it. This would require changing some base classes to store the best move count and the last accepted step move count.

Does this become the new metric by which we measure solver performance, and therefore is it enabled by default? If so, documentation needs to change, replacing uses of score calculation speed by move speed.

I believe the primary metric remains score calculation speed, and we should delegate the comparison of move speed to the benchmark module. With that in mind, the performance page will not change significantly except for adding a reference to the new metric. It doesn't make sense to me to compare a configuration using R&R with one that doesn't. Hence, when comparing two configurations using R&R, the score calculation still makes sense for evaluating performance.

[Christopher-Chianelli]

I am okay with the move evaluation speed name, but I am concern that one of the tests, where Ruin moves are not used, have different counts for score calculation count and move evaluation counts (which smells like a bug). Otherwise LGTM.

[triceo]

I couldn't find any useful sub-metric for the move count. We already have the move count per step.

Some other metrics can track their values per-move. So, for example, you could have "move execution count for change moves" etc. This is what I mean. There is precedent.

Solver logging additionally includes move speed (move count over time), along with score speed which it already does.

I initially included it but then removed it. My point is that it may confuse the user as it is the same number when not using R&R moves. I believe that the benchmark should be used to compare move speeds, and the score calculation speed should still be the primary metric.

Good point on it being the same number in many cases.
Not sure on what the default should be; arguably, score calc speed is now the confusing one, because based on different moves, it changes. Move execution speed is the one that's "stable".

There is a score count related type of Termination. We need one of those for move count as well.

I considered adding unimprovedMoveCount, but I'm unsure if it's worth it. This would require changing some base classes to store the best move count and the last accepted step move count.

I do think it is worth the effort, for the same reason as I said above. Score calculation count is not stable, its definition changes based on moves used. Move execution count is stable and always means the same thing.

[triceo]

LGTM after the suggested changes are applied.

[triceo]

There was something still bugging me about this PR. So I opened the IDE and looked at it. I know what it was now. Disabling metric collection is a very specific thing, it only ever exists for RnR. Yet, it proliferates everywhere - to every phase, to every step, everywhere.

I fixed it by only dealing with disabling metric collection when already in RnR. If we ever need it in more phases than here, we can generify it - but right now, the correct decision was to not introduce this anywhere but in RnR.

I have similarly updated the enterprise PR, which no longer needs to check whether metrics are enabled. As long as RnR doesn't trigger any metrics, nobody else (not LS, not PS, ...) needs to know anything about metrics being enabled or disabled.

Please take a look and check my thinking.

[zepfred]

There was something still bugging me about this PR. So I opened the IDE and looked at it. I know what it was now. Disabling metric collection is a very specific thing, it only ever exists for RnR. Yet, it proliferates everywhere - to every phase, to every step, everywhere.

I fixed it by only dealing with disabling metric collection when already in RnR. If we ever need it in more phases than here, we can generify it - but right now, the correct decision was to not introduce this anywhere but in RnR.

I have similarly updated the enterprise PR, which no longer needs to check whether metrics are enabled. As long as RnR doesn't trigger any metrics, nobody else (not LS, not PS, ...) needs to know anything about metrics being enabled or disabled.

Please take a look and check my thinking.

Looks good to me!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
75.5% Coverage on New Code
0.8% Duplication on New Code

See analysis details on SonarCloud