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: Revised, increased speed #1097

Merged
merged 6 commits into from
Nov 10, 2017
Merged

Conversation

nikamirrr
Copy link
Contributor

Reworked the solution, eliminated few inefficiencies

Reworked the solution, eliminated few inefficiencies
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.

Just a minor change. Great work!

for let, dig in zip(t_lowdigs, t_digvals)])
if testsum % 10 == 0:
# Low digit test passed, check the main digits
# print(lowdigdict)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove print() statements (even commented out).

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.

LGTM, thanks!

@nikamirrr
Copy link
Contributor Author

I think we can do even better, like a recursive algorithm to check the lowest digits,
move to next digits, generate them and check them.
A carryover will be accumulated for the next digits.
I am going to try to implement it over the weekend. Yesterday attempt was not very successful.
I may also try to design a special test case, which will cripple down the inefficient code.

@nikamirrr
Copy link
Contributor Author

Since the pull request is approved. Is there anything else I need to do on my side? Please, let me know

@cmccandless
Copy link
Contributor

cmccandless commented Nov 10, 2017

@nikamirrr Regarding a recursive solution, it may not be as fast as you think. From this article by Lars Kruse (found on the Python Wiki):

Function call overhead in Python is relatively high, especially compared with the execution speed of a builtin function.

If you are going to attempt another refactor, please just continue to use this PR. If you don't plan on adding work to this iteration, it should be good to merge.

@nikamirrr
Copy link
Contributor Author

I understand the general impact of the recursion. But if it allows to drop bad paths early enough it will be beneficial

@cmccandless
Copy link
Contributor

@nikamirrr Fair enough. I'm curious to see how it turns out.

@N-Parsons
Copy link
Contributor

Looks like run time is down to 0.5 seconds now compared to ~5 seconds with the previous iteration.

@N-Parsons N-Parsons changed the title Revised, increased speed alphametics: Revised, increased speed Nov 10, 2017
Copy link
Contributor

@N-Parsons N-Parsons left a comment

Choose a reason for hiding this comment

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

@nikamirrr, if you're planning to do any more work on this, let me know, but I think this is fine to merge as is.

@nikamirrr
Copy link
Contributor Author

Please, merge as is. Any further improvement is experimental and a bit less likely.

@cmccandless cmccandless merged commit 0d9d9e7 into exercism:master Nov 10, 2017
@cmccandless
Copy link
Contributor

@nikamirrr Merged. Thanks!

smalley pushed a commit to smalley/python that referenced this pull request Nov 12, 2017
smalley pushed a commit to smalley/python that referenced this pull request Nov 12, 2017
@nikamirrr
Copy link
Contributor Author

@cmccandless
Looks like the recursive solution is faster.
The function call overhead is compensated by the fact that a good found candidate is traced through the next levels immediately and bad ones are rejected faster and do not build up.

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.

3 participants