Implemented several infinite families of named graphs #415
Conversation
There was a problem hiding this comment.
Lots of good stuff.
After a while I stopped adding comments about IsPosInt, but imagine they continue.
All of these new graphs now need documentation (:grimacing:)
In the immutable constructors it would be nice to add any obvious properties that the graphs have.
gap/examples.gi
Outdated
| if k > n then | ||
| Error("the argument <n> must be greater than or equal to <k>,"); | ||
| fi; | ||
| permList := PermutationsList([1 .. n]); |
There was a problem hiding this comment.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
wilfwilson
force-pushed
the
ben/name-graph-fusene
branch
from
477c9e9
to
6839975
Compare
wilfwilson
force-pushed
the
ben/name-graph-fusene
branch
2 times, most recently
from
c5e2f03
to
a60a4c3
Compare
There was a problem hiding this comment.
Thanks for doing all of this! I have recently squashed and rebased all of your commits, and resolved the many (many!) merge conflicts in the process. It should therefore now be easy to make further changes to this, and to see what is failing on the tests.
(I recommend in the future to do a larger number of smaller PRs; each one is quick and easier to create, review, and get merged, and there is a smaller scope for merge conflicts.)
Since this PR is very large, I haven't given it a completely thorough review, but I note that @flsmith has also given this a bit of a review, and perhaps other people are going to, as well. But I've given it a bit of a skim and added some specific comments.
Unfortunately, if other PRs are merged before this one, it's likely that there will be more merge conflicts. If you need help resolving these, just let one of us know. (Note that my resolution of the previous conflicts evidently didn't go perfectly, since now there are some methods which are no longer adjacent in the file, even though they presumably used to be).
wilfwilson
force-pushed
the
ben/name-graph-fusene
branch
from
80f5516
to
7dd6b92
Compare
|
Thanks for your further work on this @bspiers, sorry for the further delay in reviewing. I've now squashed and rebased and resolved merge conflicts, and organised the new contributions into (internal) alphabetical order. Let's see if the tests pass now. I haven't looked at the implementations in detail, but I'll give the code a quick look again soon (but on the whole your implementations certainly seem reasonable). |
There was a problem hiding this comment.
I started making suggestions in the GitHub interface about tagging named arguments in the documentation with <A> tags, both when they are freestanding, and when they are in code/maths (which requires using <C> tags rather than <M> in these cases).
I will do the rest directly in the code, it's much easier.
* AndrasfaiGraph * BinomialTreeGraph * BondyGraph * CirculantGraph * CycleGraph * GearGraph * HalvedCubeGraph * HanoiGraph * HelmGraph * HypercubeGraph * KellerGraph * KneserGraph * LindgrenSousselierGraph * MobiusLadderGraph * MycielskiGraph * OddGraph * PathGraph * PermutationStarGraph * PrismGraph * StackedPrismGraph * WalshHadamardGraph * WebGraph * WheelGraph * WindmillGraph
wilfwilson
force-pushed
the
ben/name-graph-fusene
branch
from
b7ae02c
to
f36784e
Compare
There was a problem hiding this comment.
I'm happy to approve at this point (assuming the tests pass), thanks for all your hard work @bspiers
There are further suggestions that I could make in the documentation to make it more "Digraphs-like", such as adding references to related functions, and perhaps using slightly different language. But that's quite a subtle thing to do right, so I'm happy for a more experienced Digraphs contributor take that on in the future, if we ever feel like it.
wilfwilson
dismissed
flsmith’s stale review
I think there's been reasonable changes.
Implemented some new graph families and added 'wrappers' for some existing named graphs to preserve backwards compatibility.