Speed up OnDigraphs for a digraph and a permutation
#267
Conversation
There was a problem hiding this comment.
Thanks for noticing this @ChrisJefferson. I made a couple of comments - I'll give @james-d-mitchell a chance to say what he thinks, and see how attached he is to the error message.
| local out; | ||
| if ForAll(DigraphVertices(D), i -> i ^ p = i) then | ||
| local out, permed; | ||
| if p = () then |
There was a problem hiding this comment.
In one sense this is worse: previously we were testing whether p acts as the identity on DigraphVertices(D), which can be the case even if p is not the identity permutation. But I don't mind too much about losing that case.
Is p = () optimised somehow? Do all permutations inherently know whether or not they are the identity, for instance?
There was a problem hiding this comment.
P=() isn't specifically optimised, but it will almost always be very fast, espically if (as is common) the permutation moves the largest point it is defined on, as that is checked first.
There was a problem hiding this comment.
Just to say, we could use SmallestMovedPoint is we really care about saving this optimisation. That is still much faster than the old code, and would keep the same answer.
| permed := Permuted(out, p); | ||
| if Length(permed) > DigraphNrVertices(D) then |
There was a problem hiding this comment.
If p is invalid, Permuted simply won't work properly (I think) and cause its own Error to happen, which on the one hand is good because it means that the method prevents something improper happen, but it does mean that we lose the ability to give a proper error message.
|
Another solution is that we could create some |
|
@ChrisJefferson I know this is a bit late (sorry) but I intend to come up with a resolution for this soon that keeps the performance improvement. |
wilfwilson
changed the title
wilfwilson
changed the title
OnDigraphs for a digraph and a permutation
wilfwilson
force-pushed
the
ondigraphs-speed
branch
from
31f65c6
to
2b23a40
Compare
* Instead of checking if the permutation fixes the vertices, check if it is the identity (which is much cheaper, although catches less cases) * Do not check explictly if the permutation maps the vertices of the graph to itself, do the mapping then check afterwards.
wilfwilson
force-pushed
the
ondigraphs-speed
branch
from
2b23a40
to
473853b
Compare
|
Stale pull request message |
This came up from some code which was heavily using digraphs. this almost doubled the speed of the algorithm, as it turns out these GAP-level checks were dominating the run-time of OnDigraphs.
Instead of checking if the permutation fixes the vertices, check
if it is the identity (which is much cheaper, although catches
less cases)
Do not check explictly if the permutation maps the vertices
of the graph to itself, do the mapping then check afterwards.