-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
Reworked the solution, eliminated few inefficiencies
There was a problem hiding this 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!
exercises/alphametics/example.py
Outdated
for let, dig in zip(t_lowdigs, t_digvals)]) | ||
if testsum % 10 == 0: | ||
# Low digit test passed, check the main digits | ||
# print(lowdigdict) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I think we can do even better, like a recursive algorithm to check the lowest digits, |
Since the pull request is approved. Is there anything else I need to do on my side? Please, let me know |
@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):
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. |
I understand the general impact of the recursion. But if it allows to drop bad paths early enough it will be beneficial |
@nikamirrr Fair enough. I'm curious to see how it turns out. |
Looks like run time is down to 0.5 seconds now compared to ~5 seconds with the previous iteration. |
There was a problem hiding this 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.
Please, merge as is. Any further improvement is experimental and a bit less likely. |
@nikamirrr Merged. Thanks! |
@cmccandless |
Reworked the solution, eliminated few inefficiencies