Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alphametics exercise updated #1119

Merged
merged 12 commits into from
Dec 21, 2017
Merged

Alphametics exercise updated #1119

merged 12 commits into from
Dec 21, 2017

Conversation

nikamirrr
Copy link
Contributor

@nikamirrr nikamirrr commented Nov 23, 2017

Let's cripple the brute force solutions. :)

Recursive solution, seems to be the fastest
Added a longest test case
More optimal, no ifs, faster :)
Some fine tuning....
Probably a minor enhancement
@cmccandless
Copy link
Contributor

@nikamirrr I'm hesitant to add such a test on a whim. Currently this exercise is listed as a difficulty 6 in config.json; this new test would certainly raise that, as brute-force solutions could easily take minutes to solve the 10-letter test (satisfying your goal of removing them as viable solutions), and non-brute-force solutions take much more thought. I would be curious to know the opinions of those more active in the problem-specifications repo on this... would you be willing to open a PR over there to add this test? If they think adding the test is a good idea, I would be happy to merge the test here.

@nikamirrr
Copy link
Contributor Author

nikamirrr commented Nov 27, 2017

Will do, sure.

Though, I would question the real difficulty. The most difficult part is the recursive mod-10 logic.
But beyond that all 10-letter problems are the same. 10! (3628800) brute force complexity.
All you need to do in order to make all 10-letter the same is to group and count all letters on both sides in the same decimal rank.
With little thought one can change a scary looking expression into the no more that 10-pair dictionary: letter -> multiplier to check the digit permutations.

Since you already have 10-digit problems among the tests, I don't think the one I add really increases complexity. But it will be very pedagogical stimulating the thought how one can more efficiently count the repeating letters.

@nikamirrr
Copy link
Contributor Author

My point is that if the current solutions for 10-letter problems are acceptable, it does not take much to make them handle the test I add. All one need to do is to group the letters on each side.

@cmccandless
Copy link
Contributor

But beyond that all 10-letter problems are the same.

Allow me to suggest that you are almost correct; if N is the number of unique letters in the problem, all problems will have O(N!) time complexity, so problems with equal N will be the same in that regard.

However, consider if M is the total number of letters in the problem. Because of the need to check each candidate solution against the actual problem (requiring M substitutions each time), our actual time complexity is O(N! x M), which will be vastly different between a problem with 31 total letters and a problem with 10000 total letters.

@nikamirrr
Copy link
Contributor Author

You do not need to count every letter individually. No matter how many letters you use, there are no more than 10 unique letters (unknowns) with 10 non-repeating options for each unknown
For example one can notice:
NO + NO + TOO == LATE
NO + NO + TOO - LATE == 0

10N + O + 10N + O + 100T+10O+O == 1000L + 100A+10*T+E

20N + 13O + 90T - 1000L - 100*A - E == 0

So the complexity is O((10 + 1)!)

@nikamirrr
Copy link
Contributor Author

Anyway, I am going to pull the test out of the PR and suggest a solution only. The test case will go through the JSON

@cmccandless
Copy link
Contributor

cmccandless commented Nov 27, 2017 via email

nikamirrr added a commit to nikamirrr/problem-specifications that referenced this pull request Nov 27, 2017
A test case update suggested to complement this PR:
exercism/python#1119
@nikamirrr
Copy link
Contributor Author

nikamirrr commented Nov 27, 2017

NP at all :) we are all having fun doing this.
In my best faith, the idea to group the letters should be very appropriate for this levels of the exercise.

Copy link
Contributor

@cmccandless cmccandless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, some commenting would be nice, but perhaps not completely necessary for an example solution.

return dict(zip(t_lowdigs, t_digvals))
totchars = set()
for p, chardict in enumerate(tchars):
for c, cnt in tuple(chardict.items()):
Copy link
Contributor

@cmccandless cmccandless Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should run fine without wrapping chardict.items() in tuple().
This is fine.

@nikamirrr
Copy link
Contributor Author

nikamirrr commented Nov 28, 2017

Let's keep it open till next week. I will add comments. :)
Also, I have one more idea how to speed it ip.

@cmccandless with all my respect, I see why you propose running without chardict.items() in tuple().
I had it without tuple() originally and it works in python2.7
But python3.6 throws a Runtime Error when I clean-up 0-count letters from the dictionary in that loop.
so tuple() effectively creates an immutable copy of the dictionary data to iterate over.

If possible, please. check this behavior in python 3.6 on your side and let me know how chardict.items() works without tuple()

Traceback (most recent call last): File "alphametics_test.py", line 16, in test_invalid_leading_zero_solution self.assertEqual(solve("ACA + DD == BD"), {}) File "F:\devel\alphametics.py", line 83, in solve for c, cnt in chardict.items(): RuntimeError: dictionary changed size during iteration

@cmccandless
Copy link
Contributor

@nikamirrr My mistake; when I remarked on the tuple() wrap, I was only doing a cursory line-by-line reading, and I didn't connect that you were deleting items from the dictionary while iterating through it. I've corrected my comment.

@nikamirrr
Copy link
Contributor Author

NP, we have time, I will add comments over the weekend and update PR for the review.
I should have commented the code, you are exactly right. I am not a fulltime pro developer, so documenting the code was never a requirement for me, which I know is not right )

Added comments
@nikamirrr
Copy link
Contributor Author

Comments added

Stargator pushed a commit to nikamirrr/problem-specifications that referenced this pull request Dec 19, 2017
Stargator pushed a commit to exercism/problem-specifications that referenced this pull request Dec 19, 2017
@stale
Copy link

stale bot commented Dec 20, 2017

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the abandoned label Dec 20, 2017
@nikamirrr
Copy link
Contributor Author

Hello, would anybody pull this, please?

@stale stale bot removed the abandoned label Dec 21, 2017
@cmccandless cmccandless merged commit ede1ad7 into exercism:master Dec 21, 2017
@cmccandless
Copy link
Contributor

@nikamirrr apologies, I was waiting for exercism/problem-specifications#1024 to be merged first, then I forgot about it. Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants