palindrome-products: Update palindrome-products to reflect changes to canonical data

[emerali]

Resolves #1000

[emerali]

I realized that the example solution doesn't completely solve the problem (which was also the case for the old version), which also renders some of the tests useless.

I'll fix it tomorrow.

[emerali]

As mentioned in #1052, assertRaisesRegexp is deprecated in Python 3, as it was renamed to assertRaisesRegex in 3.2 (thus assertRaisesRegex is not available in Python 2.7).

[cmccandless]

Pending discussion in #1080

[cmccandless]

(shameless copy-paste from #1052) @emerali Thank you for your patience on this. I'd say for now remove all checks for exception messages. If the conclusion in #1080 calls for checking them, a new pull request will be made to add them (other exercises will likely require similar changes anyway).

[emerali]

sorry about that, i'd forgotten about this PR, thanks for the reminder 😄

i've removed the error messages, please take a look

[emerali]

@cmccandless i realized that i probably should have mentioned you in this thread

[cmccandless]

Not sure I agree with the usage of nested sets here... the canonical data expects each element of factors to be a pair (tuple, list, etc), and returning as a set makes duplicate factor pairs into singletons. I would suggest the following form instead:

expected = {(1, 9), (3, 3)}

[emerali]

Sets were used to be able to compare individual tuples without having to worry about the orders of their elements, so, for example, (1, 9) and (9, 1) would be equivalent.

An alternative may be to sort the elements of each tuple in the set, what are your thoughts on that?

[cmccandless]

There's precedence for sorting tuple elements, actually: see the normalize_dominoes function in the dominoes exercise. It may not be exactly what you need but would get you pretty close. That function also would catch duplicate pairs, since it works with lists instead of sets, but modifying it to use sets wouldn't be hard.

[emerali]

@cmccandless I've made the changes you requested, but I've noticed that the tests run a bit slow.

I'll look into optimizing the example solution over the weekend.

[cmccandless]

@emerali Looking at the significant increase in test execution time, I've begun to rethink using sets. Still not a fan of explicitly using frozenset in the expected results as it decreases readability. More importantly, I still think it is much more natural for a user to be able to return tuples or lists over sets. Perhaps something like the following would run faster?

def test_largest_palindrome_from_single_digit_factors(self):
    value, factors = largest_palindrome(min_factor=1, max_factor=9)
    self.assertEqual(value, 9)
    self.assertFactorsEqual(factors, {(1, 9), (3, 3)})

def assertFactorsEqual(self, actual, expected):
    self.assertEqual(set(map(frozenset, actual)), set(map(frozenset, expected)))

This would allow the user to return the collection type of their choice, while simultaneously allowing expected results to be specified in a more readable format. Of course, this is only an improvement if it actually does run faster...

[emerali]

@cmccandless I tried changing the assertions in the way that you had suggested, but it still didn't seem to make much of a difference to performance overall.

I did a bit of profiling and realized that it was mostly the palindrome generation that was using up most of the time, since it was basically generating every value in a range and then checking if they were palindromes, individually.

I've optimized the palindrome generation, and now it runs a lot faster 😄

[cmccandless]

@emerali Merged; thanks!