Skip to content

Improves backwards compatibility in alphametics after addition of Combinatorics.jl#1055

Merged
depial merged 2 commits intoexercism:mainfrom
depial:alphametics-back-compat
Dec 6, 2025
Merged

Improves backwards compatibility in alphametics after addition of Combinatorics.jl#1055
depial merged 2 commits intoexercism:mainfrom
depial:alphametics-back-compat

Conversation

@depial
Copy link
Contributor

@depial depial commented Dec 6, 2025

In response to suggestion by @cmcaine here.

  • Restores permutations.jl to editor files, but with the code commented out so as to avoid any namespace conflicts.
  • File can be used locally, with more details in instructions.append.md on how to use permutations(a, t) in all cases.
  • Replaced brute force algorithm in example.jl with more performant pruned dfs approach.
  • using Combnatorics has been changed to using Combinatorics: permutations to 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.jl will be readable to all students, albeit it will be lacking syntax highlighting since the code is commented out.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

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.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

@depial
Copy link
Contributor Author

depial commented Dec 6, 2025

Interestingly, Exercise CI ran much faster than it has been.

It appears that using Combinatorics is significantly slower than just using the function directly from permutations.jl.

For example, my brute force solution which passed with permutations.jl, now times out on the site with Combinatorics.jl. I'm unsure what would be causing this significant change in performance.

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.

@depial depial requested review from cmcaine and colinleach December 6, 2025 18:36
@depial
Copy link
Contributor Author

depial commented Dec 6, 2025

At present, without knowing exactly why there has been such a slow down with Combinatorics.permutations(), the immediate solutions that come to mind are:

  • Remove the final two tests from runtests.jl
  • Revert the changes, and forgo using Combinatorics.jl

I see this issue locally as well. Indeed, the original example.jl appears to be more performant that the dfs implementation when using permutations.jl, but this flips when using Combinatorics.jl (presumably since permutations() isn't called in the dfs).

@colinleach
Copy link
Contributor

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...

@depial
Copy link
Contributor Author

depial commented Dec 6, 2025

The code in the trimmed down permutations.jl is effectively the same as what is in Combinatorics.jl/src/permutations.jl, with the exception of the factorial definition for the Base.length() method and some cherry picking to cut out things related to MultiSetPermutations.

So far I've tried:

  • For factorial difference: Used the definition for Base.length() that is in source in the trimmed down version => no noticeable change in performance.
  • For the cherry-picking difference: Copied and pasted the entire file from source and used it locally => Slower on first run than the trimmed down version, but still reasonable, same performance on further runs.

This second point is what I find so confusing. Why would calling the function from Combinatorics.jl be so much worse than using the same code defined locally? I had first suspected this was a problem with how the test runner interacts with the package, but now it appears to be a wider issue since I'm reproducing it locally. Can you reproduce this behavior on your machine too?

Comments out the last two tests with ten letters due to issues with Combinatorics.jl
@depial
Copy link
Contributor Author

depial commented Dec 6, 2025

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 permutations().

@depial
Copy link
Contributor Author

depial commented Dec 6, 2025

Just some quick benchmarks for:

@time solve("AND + A + STRONG + OFFENSE + AS + A + GOOD == DEFENSE")

With using Combinatorics: permutations

  • First run: 80.253628 seconds (5.48 M allocations: 306.701 MiB, 0.05% gc time)
  • Subsequent: 81.658769 seconds (5.48 M allocations: 306.701 MiB, 0.04% gc time)

Copy-and-pasting source code:

  • First run: 0.364985 seconds (14.64 M allocations: 948.861 MiB, 29.09% gc time, 6.16% compilation time)
  • Subsequent: 0.385775 seconds (14.62 M allocations: 947.979 MiB, 29.06% gc time)

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 Combinatorics.jl.


Aside
With dfs:

  • First run: 0.242401 seconds (264.27 k allocations: 12.829 MiB, 97.41% compilation time)
  • Subsequent: 0.006320 seconds (37 allocations: 2.000 KiB)

So we might want to keep this as the example.jl in the end anyway.

@depial depial merged commit fad064b into exercism:main Dec 6, 2025
5 checks passed
@depial depial deleted the alphametics-back-compat branch December 6, 2025 23:44
@cmcaine
Copy link
Contributor

cmcaine commented Feb 15, 2026

Did you ever find out what was going on here, or why permutations() was performing so badly?

@depial
Copy link
Contributor Author

depial commented Feb 15, 2026

There was an ill-advised change to the algorithm back in 2023. This killed the performance of Combinatorics.permutations() on larger inputs. There was new version released (v1.1.0) in mid-December, which has a roundabout correction, but it's still not as performant as the version you had put in permutations.jl in the alphametics exercise.

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).

@depial
Copy link
Contributor Author

depial commented Feb 15, 2026

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.

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