Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up OnDigraphs for a digraph and a permutation #267

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ChrisJefferson
Copy link
Contributor

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.

Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

@wilfwilson wilfwilson Oct 28, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +617 to +675
permed := Permuted(out, p);
if Length(permed) > DigraphNrVertices(D) then
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@wilfwilson
Copy link
Collaborator

Another solution is that we could create some NC versions of functions that are particularly performance-critical, including OnDigraphsNC.

@wilfwilson
Copy link
Collaborator

@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 wilfwilson changed the title Speed up OnDigraphs for a graph and a permutation Speed up OnDigraphs for a digraph and a permutation Mar 3, 2021
@wilfwilson wilfwilson changed the title Speed up OnDigraphs for a digraph and a permutation Speed up OnDigraphs for a digraph and a permutation Mar 26, 2021
* 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.
@github-actions
Copy link

github-actions bot commented Jan 7, 2022

Stale pull request message

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