cliques: some perf improvements #635
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
james-d-mitchell
force-pushed
the
cliques-perf
branch
2 times, most recently
from
1801b31
to
1d124f0
Compare
james-d-mitchell
added
the
performance
james-d-mitchell
force-pushed
the
cliques-perf
branch
2 times, most recently
from
59f7be3
to
36be423
Compare
|
TODO: this means we don't need Orb for Digraphs any more. |
reiniscirpons
previously approved these changes
Apr 8, 2024
gap/cliques.gi
Outdated
| Add(orbits, orb); | ||
| Append(out, orb); | ||
| if not c in orbits then | ||
| MultiOrbit(G, c, orbits); |
There was a problem hiding this comment.
This line is a bit confusing, since MultiOrbit returns an orbit and its non-obvious that it changes the orbits hashmap in place (this is what its used here). Would it make sense to just inline the bit of the MultiOrbit code thats relevant to updating the orbits hashmap?
There was a problem hiding this comment.
I'm not sure it makes sense to inline the bit of code in MultiOrbit, it is used elsewhere, and then we'd have duplication of the method. Maybe I can rename the function to AddOrbitToMap or something?
james-d-mitchell
force-pushed
the
cliques-perf
branch
from
36be423
to
0f99b3a
Compare
This commit includes some performance improvements for the clique
finding code. The example in the newly added extreme/cliques.tst file
takes approx. 4s with the modified code, versus at least 3 minutes
previously (I never ran it to completion previously because it was
taking too long). If we do:
List(DigraphCliquesReps(D),
x -> Orbit(AutomorphismGroup(D), x, OnSets));
for the same example, then this takes about 12s, doing the same with
List(DigraphCliquesReps(D),
x -> Orb(AutomorphismGroup(D), x, OnSets));
instead runs out of memory repeatedly (and takes longer than 12s). Note
that the automorphism group is of size 16, and so the orbits are
relatively small in comparison to the number of cliques.
james-d-mitchell
force-pushed
the
cliques-perf
branch
from
0f99b3a
to
4318a25
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit includes some performance improvements for the clique finding code. The example in the newly added extreme/cliques.tst file takes approx. 4s with the modified code, versus at least 3 minutes previously (I never ran it to completion previously because it was taking too long). If we do:
for the same example, then this takes about 12s, doing the same with
instead runs out of memory repeatedly (and takes longer than 12s). Note that the automorphism group is of size 16, and so the orbits are relatively small in comparison to the number of cliques.