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

Use the new graphviz package #622

Closed
wants to merge 3 commits into from
Closed

Use the new graphviz package #622

wants to merge 3 commits into from

Conversation

mpan322
Copy link
Contributor

@mpan322 mpan322 commented Mar 5, 2024

Notes:

  • I tried to limit the number of changes I made to the base code as much as possible (no need to add bugs if possible).
  • This likely results in some redundant code (TODO).
  • Tests for the code have been updated, but they have been checked that they are still testing the same examples.
    • Minor Note Previously graphs in the new code were laid out differently than graphs in the old code. Following recent changes to graphviz (adding an ordering in output based on the order they were added to the object) this is no longer the case. They now match up directly.
  • When writing these I added GV_<old name> methods for testing. Let me know if you would like these removed.
    • There should be a <old name> equivalent of GV_<old name> which do the exact same thing.

@james-d-mitchell
Copy link
Member

Nice! @mpan322 the ci is failing because it doesn't know how to install the graphviz package. That should be easy to fix (in the morning!)

@james-d-mitchell james-d-mitchell changed the title New version of displaying defaults added to the digraphs pacakge using the graphviz package. The Splash operation was also moved into the main grpahviz package. Use the new graphviz package Mar 7, 2024
@james-d-mitchell james-d-mitchell added the new-feature A label for new features. label Mar 7, 2024
@james-d-mitchell
Copy link
Member

@mpan322 it looks like the ci now does (mostly) install the graphviz package, but there are a few issues:

  1. The linting fails for the files you've modified in the Digraphs package
  2. It seems that graphviz uses Pluralize which was introduced in GAP 4.12, so we should probably not use Pluralize in graphviz, and ensure that the graphviz package requires GAP 4.10 or higher (I'm not sure what it requires ATM but it must not require GAP 4.12 because it wouldn't have loaded in the CI jobs using earlier versions of GAP).

There might be other issues with graphviz not being installed in some of the CI jobs, I can fix those, probably best to fix 1 and 2 above first though. Thanks again!

@james-d-mitchell james-d-mitchell added the waiting for creator input A label for issues/PRs where we are waiting for the creator to do something label Mar 13, 2024
@mpan322 mpan322 deleted the branch digraphs:main March 22, 2024 17:13
@mpan322 mpan322 closed this Mar 22, 2024
@mpan322 mpan322 deleted the main branch March 22, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature A label for new features. waiting for creator input A label for issues/PRs where we are waiting for the creator to do something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants