Change: Add test for incorrect greedy algorithm

[dbalmain]

I'd like to propose adding a missing edgecase test. This test invalidates an incorrect greedy algorithm which takes as many of the larger coins as possible as long as the remainder is greater than or equal to the value or the smallest coin. Currently such an algorithm will pass all of the tests.

If required, I can point to some solutions which fail this test.

[stkent]

change with Lilliputian Coins should already fail if the greedy algorithm is used, I think?

[dbalmain]

Take a look at this greedy algorithm here.

[stkent]

Interesting! I ran this test case against the Java and Kotlin track example solutions; Java passed while Kotlin failed!

Given that, this is definitely a worthy addition. If possible I'd like for us to better understand exactly how this case differs from change with Lilliputian Coins (i.e., what makes it possible for change with Lilliputian Coins to pass while this new test fails) and then encode that knowledge in the canonical data test name or description. Sound reasonable?

[dbalmain]

The Kotlin example solution has an error on line 48. For some reason it's converting a bad result (ie A NO_COINS_INDICATOR used to indicate that no solution was found) into an empty solution with the safeResult() call. I can't see why this was added but if you remove it, everything is fine.

Although this test catches a problem with the Kotlin algorithm, it's not really a good example of what the test was intended to catch. It's testing greedy algorithms that will assume they can continue to use larger coins as long as the remainder is still greater than or equal to the value of the smallest coin. I think the solution I linked to earlier is a better example of this.

Perhaps we could simply call the test test_incorrect_greedy_algorithm?

[petertseng]

So is the distinguishing factor this: Unlike "possible change without unit coins available", at the point at which you must stop using some coin C, the remainder still exceeds the coin C + the smallest coin.

[dbalmain]

@petertseng, yes, that's good way to express the difference.

[stkent]

I'll need to return to this with a clearer head in the morning to make sure I understand the distinction; thanks for the input, all!

[stkent]

Gotcha, this makes sense! So that's why change with Lilliputian Coins doesn't exercise everything the newly-added test does; because when you stop using the coin 15, the remainder (8) does not exceed 16 (= 15 + 1)?

[petertseng]

when you stop using the coin 15, the remainder (8) does not exceed 16 (= 15 + 1)?

Correct. Something that makes me curious... what about Lower Elbonia coins? At the point at which you must stop using 25, the remainder (63) is certainly larger than 25 + 1. I was wondering whether the greedy algorithm would thus also fail on that case, but maybe some implementations have a provision for when the remainder is exactly a multiple of some coin. So I do like the addition of the proposed case. Sorry I can't Approve in the GitHub sense of the word via email (I have attempted), but I approve the colloquial sense.

[stkent]

Using Lower Elbonia coins 1, 5, 10, 21, 25 to target 63, a greedy algorithm would first use 2 x 25 with remainder 13 < (25 +1), I think?

In any case, with a couple approvals I'm going to go ahead and merge this. Thanks for the add!

[petertseng]

Were you wanting (and do you still want) a description that describes the difference, rather than simply "another"?

[stkent]

I think that's a nice to have, given that the other test cases for this problem are also description-less. But I wouldn't turn down a PR that added them throughout, that's for sure!

[petertseng]

what about Lower Elbonia coins? At the point at which you must stop using 25, the remainder (63) is certainly larger than 25 + 1

I missed a distinguishing factor of the incorrect algorithm: It knew to check the smallest 1 coin, the smallest 2 coins, the smallest 3 coins ... all the coins.

So a test case needs to require the use of the largest coin, which is true of the [4, 5] case that was added in this PR (5 is the largest coin and it is used), and was not true of Lower Elbonia (largest coin was 25, and it is not used in the solution) which existed before this PR.