Improve DenseNautyGraph implementation

[mxhbl]

This PR greatly improves the basic implementation of the DenseNautyGraph graph type and is the first of a series of PRs that will work towards this bounty: JuliaGraphs/Graphs.jl#452.

Main Changes

The low-level implementation of DenseNautyGraph has been simplified and DenseNautyGraph now satisfies the Graphs.jl API. The dense graph format used by nauty stores the full adjacency matrix of the graph as a bit matrix. This bit matrix type is now mirrored on the Julia side in the form of the Graphset type, which behaves similarly to the standard Julia BitMatrix, but is not the same (the name "graphset" is more or less borrowed from the nauty manual). The internal storage format of the bits is different, and Graphset is resizable, so that the number of graph vertices can be changed. Most high level Graph.jl methods on a DenseNautyGraph are now implemented via a corresponding low-level method acting on the underlying Graphset. There is also some minimal documentation added to the constructors of DenseNautyGraph.

Questions & Issues

I don't know what level of detail the review should go to. Below is a list of questions or issues I have encountered, roughly in order of relevance.

• Subtleties of the Graphs.jl interface.
  Correct supertype: Right now, I define DenseNautyGraph <: AbstractNautyGraph{Int} <: AbstractGraph{Int}, since the vertices of a DenseNautyGraph are Int indices into the underlying adjacency matrix. Is this the correct choice? Is there anything to be gained from inheriting from AbstractSimpleGraph?
  Edge iterators: It is a bit unclear to me how strictly the behavior of edges(g) has to be defined. For now, I took a rather strict approach and I implemented edges(::NautyGraph) by defining methods for SimpleEdgerIterator{<:NautyGraph}. This should lead to maximum compatibility, but is a bit cumbersome. A more lightweight approach would be to simple return an iterator over the nonzero elements of the adjacency matrix.
• Should vertex labels be part of the (dense)nautygraph type, or should they be passed to nauty separately? I think there are advantages and downsides to both approaches. Even though the design philosophy of Graphs.jl seems to be that any metadata should be separate, I think that especially in the context of isomorphism checking, vertex labels can be considered part of the graph object itself. This is done in the current implementation, but I am more than happy to change that. There are also some more technical issues related to memory management that are impacted by this choice, but those should be discussed separately.
• Docs: I think NautyGraphs is still lightweight enough to get by with a readme and basic doc strings; but maybe adding a full documentation site would be beneficial?
• Interoperability: Should I aim to be compatible with other packages in the Graphs ecosystem? Should I add conversion methods from e.g. MetaGraphsNext?
• Should the choice of integer type used to store the bits of the adjacency matrix be exposed to the user? The new implementation makes it possible to store the bits of the graph adjacency matrix as either UInt16, UInt32, or UInt64. I did some quick tests, and the choices does not seem to make a big performance difference. The default choice (UInt64) seems to be fastest for all graph sizes, if only slightly for small graphs (<32 vertices). So there is not really much point in using smaller integer types. On the other hand, having the option to change the integer type doesn't hurt either (and it doesn't really make the implementation more complicated), so I have left it in for now.
• GraphsInterfaceChecker.jl is unlisted, so I am not sure how to include it in the test suite. I ran it locally, and all interface tests passed.
• General style: I don't have much experience writing package code, so any general comments about style and/or organization are very welcome!

@Krastanov please let me know if this type of summary of the PR is sufficiently clear, or if other comments/descriptions are needed. This PR is a pretty big refactor of the entire package, the following PRs will probably be much more self-contained and easier to review. Also, should I give you commit/maintain rights to the repo?

[codecov]

Codecov Report

❌ Patch coverage is 87.81250% with 39 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/densenautygraph.jl	83.61%	29 Missing ⚠️
src/graphset.jl	93.13%	7 Missing ⚠️
src/nauty.jl	87.50%	3 Missing ⚠️

Files with missing lines	Coverage Δ
src/NautyGraphs.jl	100.00% <100.00%> (ø)
src/utils.jl	100.00% <100.00%> (+7.01%)	⬆️
src/nauty.jl	85.07% <87.50%> (-2.20%)	⬇️
src/graphset.jl	93.13% <93.13%> (ø)
src/densenautygraph.jl	83.61% <83.61%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Krastanov]

This is great! Here are my personal answers to topics you brought up, but plenty of what I say here is a matter of taste:

• Correct Supertype: The docs of AbstractSimpleGraph say that there is an expectation to have certain struct properties, so I think your current choice is most consistent with the prescriptions of Graphs.jl.
• Iterators: I think the simpler solution should in principle work, but who knows about hidden assumptions elsewhere in the ecosystem, so your current "defensive" more complicated solution seems like a good choice.
• Vertex labels: The Julia Graphs community has tried a few times to create good graphs packages with metadata. I believe besides metagraphs and metagraphsnext, the ITensor folks have something of their own too. I would not expect stability and support from any of these choices, so I do not have an opinion on supporting any of them. My personal opinion is that you should stick to your current choice until you see something forcing you to change that choice.
• Docs: a good readme is enough for me in the context of the bounty. I would suggest adding a section entitled something like "Developer conventions" that lists all these assumptions and reasons for structuring your code the way you have. Having a place where one can read the reasons the package was structured the way it is can really help with long term support. I would also suggest making PRs to the Graphs.jl docs to mention "wrappers around external graphs libraries that conform to the Graphs.jl API"
• Bitpacking integer type: In the context of the bounty, no need to expose that choice, but having that discussion added to the "dev conventions" bulletpoints can be pretty useful
• interface checker: You can add using Pkg; Pkg.add("github url") at the start of your runtests.jl to get libraries that are not registered. It is a hack, but it is convenient for situations like this.
• other comments: adding aqua/jet/doctests tests can help a lot. See aqua.jl, jet.jl, and doctests.jl in this library for a simple example: https://github.com/QuantumSavory/QuantumClifford.jl/

Could you email me at skrastanov@umass.edu so that I can send you the bounty processing details (for when the bounty is finished)

[mxhbl]

Thanks for the detailed feedback! I will just go ahead and merge this PR, and extend the readme, add aqua/jet, and follow your other suggestions in the coming PRs. GraphInterfaceChecker is now also included in the tests.

[Krastanov]

FYI, JET relies heavily on the compiler, and has way fewer false positives in newer versions. Generally I run it only on the latest julia version.

[Krastanov]

Another thing that might make your life easier: move all test dependencies to test/Project.toml and do not specify compat bounds for them -- that way you will have way simpler compat and way less work for the CompatHelper bot.

[mxhbl]

Yeah, I will make that change soon; managing the test dependencies on different julia versions is a bit of a headache. Thanks for the suggestions!