Skip to content

Speed up OnDigraphs for a digraph and a permutation #267

New issue

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

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

Already on GitHub? # to your account

Merged
merged 8 commits into from
Apr 10, 2025

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.

@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
@wilfwilson wilfwilson force-pushed the ondigraphs-speed branch 2 times, most recently from 2b23a40 to 473853b Compare May 23, 2021 15:20
@digraphs digraphs deleted a comment from github-actions bot Apr 7, 2025
@james-d-mitchell james-d-mitchell added the gap-days-brussels-2025 Label for things we might work on in Brussels label Apr 7, 2025
@james-d-mitchell
Copy link
Member

I'd propose improving the performance of OnDigraphs at gap days in Brussels in 2025, if we have the time.

@reiniscirpons reiniscirpons self-assigned this Apr 7, 2025
ChrisJefferson and others added 2 commits April 8, 2025 10:39
* 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.
@reiniscirpons
Copy link
Collaborator

I tried my best to incorporate the improved methods, but I just do not see the timing improvements claimed. I settled on implementing a separate OnDigraphsNC method which skips the more expensive ForAll and ForEach checks and only does the p = () optimization. I then replaced internal usage of the OnDigraphs method with OnDigraphsNC. Here are some runtime stats:

Test Old method time, ms Proposed new OnDigraphsNC method time, ms Proposed new OnDigraphs method time, ms
1000 random digraphs with 100 vertices, IsIsomorphicDigraph 703 689 -
829 random topologically sortable digraphs with 100 vertices, DigraphTransitiveReduction 65 52 -
100 random transitive digraphs with 1000 vertices, IsIsomorphicDigraph 5476 5374 -
100 random transitive digraphs with 1000 vertices, DigraphTransitiveReduction 24020 23947 -
100 random digraphs with 1000 vertices, OnDigraphs or OnDigraphsNC respectively 1415 1403 1487

There seem to be some minor improvement (within margin of error in some cases) with the proposed OnDigraphsNC method, but it slows down OnDigraphs by a bit.

@james-d-mitchell james-d-mitchell merged commit 0101a73 into digraphs:main Apr 10, 2025
27 checks passed
@james-d-mitchell
Copy link
Member

Thanks @reiniscirpons looks good

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
gap-days-brussels-2025 Label for things we might work on in Brussels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants