Improves backwards compatibility in alphametics after addition of Combinatorics.jl#1055
Conversation
|
This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested. If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos. For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping |
|
Interestingly, Exercise CI ran much faster than it has been. It appears that For example, my brute force solution which passed with There was significant lag on my machine for the last test, but even for the second to last test when using a brute force method. |
|
At present, without knowing exactly why there has been such a slow down with
I see this issue locally as well. Indeed, the original |
|
I don't have a quick answer to the performance issues. When I think about it some more, I suspect I may continue to have no good answer... |
|
The code in the trimmed down So far I've tried:
This second point is what I find so confusing. Why would calling the function from |
Comments out the last two tests with ten letters due to issues with Combinatorics.jl
|
I've commented out the last two tests (which have ten letters) as a stop-gap measure until we can figure out what is going on. This way the exercise is at least possible when using |
|
Just some quick benchmarks for: @time solve("AND + A + STRONG + OFFENSE + AS + A + GOOD == DEFENSE")With
Copy-and-pasting source code:
Beyond the notable time difference, there also appears to be a significant difference in memory allocation and garbage collection. I understand there is often a trade off between time and space, but I don't know where or how this would be handled differently with Aside
So we might want to keep this as the |
|
Did you ever find out what was going on here, or why permutations() was performing so badly? |
|
There was an ill-advised change to the algorithm back in 2023. This killed the performance of I've opened a PR there to revert these regressions while trying to further speed things up, but I was told they lack reviewers, so things don't get done very quickly (hence the time lag between the bad update in Feb 2023 and the fix in Dec 2025). |
|
Specifically, a lot of the confusion in this thread came from me using the source code in the repo, which had a "fix" but hadn't been released as a new version yet. Only when I took the source code off my machine did I see the difference. |
In response to suggestion by @cmcaine here.
permutations.jlto editor files, but with the code commented out so as to avoid any namespace conflicts.instructions.append.mdon how to usepermutations(a, t)in all cases.example.jlwith more performant pruned dfs approach.using Combnatoricshas been changed tousing Combinatorics: permutationsto keep namespace as clean as possible.This should make the previous change backwards compatible with all solutions while not complicating the namespace with multiple definitions of
permutations(a, t). Also,permutations.jlwill be readable to all students, albeit it will be lacking syntax highlighting since the code is commented out.