Skip to content

Speed up cperm#4361

Merged
fingolfin merged 2 commits intomasterfrom
mh/cperm
Dec 5, 2024
Merged

Speed up cperm#4361
fingolfin merged 2 commits intomasterfrom
mh/cperm

Conversation

@fingolfin
Copy link
Copy Markdown
Member

Also adjust the docstring. Point out that omitting the parent group is not advisable, and change uses of cperm in tests to provide a parent.

Before:

julia> g = symmetric_group(15);

julia> @btime cperm($g, 1:4,5:6,7:15);
  5.007 μs (69 allocations: 3.05 KiB)

After:

julia> g = symmetric_group(15);

julia> @btime cperm($g, 1:4,5:6,7:15);
  1.238 μs (6 allocations: 416 bytes)

There is actually some more space for improvement by adding some low level primitives for accessing and creating GAP permutations, but that's for a future PR. In the meantime this already improves things and I don't want to rebase this once again.

This was created as part of work with @Pascalonyy on Origamis from 1 year ago, for AG Weitze-Schmithüsen in Saarbrücken. Sadly that never went anywhere as far as I know, but I can at least salvage some of the bits I provided to make things much faster there.

Comment thread src/Groups/perm.jl Outdated
Comment thread src/Groups/perm.jl Outdated
@fingolfin fingolfin force-pushed the mh/cperm branch 4 times, most recently from 8ebe752 to a43c495 Compare November 28, 2024 09:31
Comment thread src/Groups/perm.jl Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.37%. Comparing base (6c0ad05) to head (733420e).
Report is 197 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4361   +/-   ##
=======================================
  Coverage   84.37%   84.37%           
=======================================
  Files         656      656           
  Lines       87190    87202   +12     
=======================================
+ Hits        73563    73575   +12     
  Misses      13627    13627           
Files with missing lines Coverage Δ
src/Groups/perm.jl 98.84% <100.00%> (+0.09%) ⬆️

... and 1 file with indirect coverage changes

@lgoettgens
Copy link
Copy Markdown
Member

Triage likes the idea that cperm delegates to a less efficient function in the case that it detects overlapping cycles.

@ThomasBreuer
Copy link
Copy Markdown
Member

Currently the documentation of @perm also states that overlapping cycles are supported, and mentions cperm.

Also adjust the docstring. Point out that omitting the parent group is
not advisable, and change uses of cperm in tests to provide a parent.

Before:

    julia> g = symmetric_group(15);

    julia> @Btime cperm($g, 1:4,5:6,7:15);
      5.007 μs (69 allocations: 3.05 KiB)

After:

    julia> g = symmetric_group(15);

    julia> @Btime cperm($g, 1:4,5:6,7:15);
      1.238 μs (6 allocations: 416 bytes)
@fingolfin
Copy link
Copy Markdown
Member Author

I have now updated this to restore support for non-disjoint cycles.

Copy link
Copy Markdown
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.
(A local variable gens is used in the tests, but this can be improved in another pull request.)

@fingolfin fingolfin merged commit 1b50847 into master Dec 5, 2024
@fingolfin fingolfin deleted the mh/cperm branch December 5, 2024 09:22
@fingolfin fingolfin changed the title Optimize cperm Speed up cperm Feb 27, 2025
@fingolfin fingolfin added optimization Simpler/more performant code or more/better tests release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes enhancement New feature or request labels Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request optimization Simpler/more performant code or more/better tests release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: groups

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants