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

Dreadnaut support #651

Open
wants to merge 55 commits into
base: main
Choose a base branch
from
Open

Conversation

pramothragavan
Copy link

@pramothragavan pramothragavan commented May 26, 2024

Broadly speaking, a dreadnaut file starts with "configuration" information about the graph, such as the number of vertices (denoted by 'n'), the start index for vertex numbering (denoted by '$') and whether or not a graph is a digraph (denoted by the presence of 'd'). The configuration section always ends with a 'g'. The rest of the file gives information concerning individual vertices in the form of adjacency lists. For example:

n=2
$=1
d
g
1: 1 2;
2: 2;

would represent a 1-indexed digraph with 2 vertices with edges {1,1}, {1,2}, {2,2}.

General overview:

Decoder:

  • DIGRAPHS_ParseDreadnautConfig aims to get values for either '$' (which indicates the start index for vertex numbering) or 'n' (which indicates the number of vertices). Note that '$' defaults to 0 and that I chose to reindex all graphs such that vertex numbering starts at one (which I think is convention for the Digraphs package?)
  • DIGRAPHS_LegalDreadnautEdge aims to filter out illegal edges and throws an error if an edge is illegal. An example of an illegal edge might be a loop for an undirected graph or an edge containing a vertex that is not allowed within the constraints of the values of '$' and 'n'. (In the case of illegal edges, nauty throws a warning message and then ignores the edge so I was trying to replicate this behaviour).
  • DIGRAPHS_SplitDreadnautLines effectively takes a line of dreadnaut (e.g. "1: 2 3 5; 4: 2 1 3; 2: 3;") and aims to split this into parts which are to be handled individually (in this case the parts would be ["1: 2 3 5;", "4: 2 1 3;", "2: 3;"]). The idea here is that although usually these parts would each be on their own line, it's techincally fine for some or all of them to share a line (with or without a semicolon) so I thought it made more sense to condense everything onto one line and then split into parts. There are various auxiliary commands that can be used within the dreadnaut format alongside the definition of the graph (more info here) which I mostly chose to neglect, with the exception of 'f' which defines a partition of vertices. Note that '$$' at the end of a file means reindex the graph to start counting at 0 (which I ignored).
  • DIGRAPHS_ParseDreadnautGraph intends to parse the non-configuration part of the file, which has been split into parts after being fed through to DIGRAPHS_SplitDreadnautLines

These are all combined in ReadDreadnautGraph.

Encoder:
WriteDreadnautGraph takes a digraph and encodes into dreadnaut format.

I'm in the process of writing documentation!

Copy link
Member

@james-d-mitchell james-d-mitchell left a comment

Choose a reason for hiding this comment

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

Generally speaking this looks really good! I've added a few comments, mostly about the adding some details to the error messages if possible. Some general comments:

  • it'd be great if the error messages could report in what line of the file the error occurs, I think we discussed this, but don't exactly remember the outcome of this. One approach would be to store the original file contents in a variable, and then search within that for the part that causes the error. I'm not sure if this would actually work or not, just a thought.
  • in the PR description you mention:

Note that '$' defaults to 0 and that I chose to reindex all graphs such that vertex numbering starts at one (which I think is convention for the Digraphs package?)

this sounds appropriate, and yes this is the convention (really more a requirement in Digraphs, i.e. at present it's only possible to have digraphs with vertices [1 .. n] for some n). It'd be best if the code at the very least issued a warning when you are renumber the vertices, to avoid violating the principal of least astonishment (i.e. try to read a graph with nodes not [1 .. n], then silently getting a graph with nodes [1 .. n] would be surprising, so better issue a warning that this is happening). It'd also be useful to have the original vertices as labels in the newly constructed digraph, so if for example the graph has 0-indexed vertices, then the labels of the vertices in the output graph would be set using DigraphSetVertexLabels(D, [0 .. n - 1]); (if this is the correct mapping).

  • You mention in a couple of other places in the PR description that your code potentially ignores some other parts of dreadnaut files, if you detect parts that are ignored for whatever reason then, please issue a warning for each of these too, again to avoid surprising the user.
  • Have you checked how good the code coverage of your tests is? Given the number of lines of code in the implementation versus the number of lines of tests, I'm guessing that there's maybe some work to do there. I've sent you a python script by email that you can use to check the code coverage, just run ./code-coverage-test-gap.py tst/standard/io.tst inside the digraphs directory.

gap/io.gi Show resolved Hide resolved
gap/io.gi Show resolved Hide resolved
gap/io.gi Outdated Show resolved Hide resolved
gap/io.gi Outdated Show resolved Hide resolved
gap/io.gi Outdated Show resolved Hide resolved
gap/io.gi Outdated Show resolved Hide resolved
gap/io.gi Show resolved Hide resolved
gap/io.gi Show resolved Hide resolved
gap/io.gi Outdated Show resolved Hide resolved
gap/io.gi Outdated Show resolved Hide resolved
@james-d-mitchell james-d-mitchell added 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 labels May 29, 2024
gap/io.gi Outdated Show resolved Hide resolved
@james-d-mitchell
Copy link
Member

@pramothragavan please let me know when you think this is ready again, and thanks !

@pramothragavan
Copy link
Author

@pramothragavan please let me know when you think this is ready again, and thanks !

Will do!

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