Open c703ac91-1c31-4f3a-abb2-47af3ac2705d opened 7 years ago
Branch: u/zgershkoff/gammoids
Commit: a861249
Branch pushed to git repo; I updated commit sha1. New commits:
a861249 | fix to rank function |
Author: Zachary Gershkoff
Description changed:
---
+++
@@ -1 +1 @@
-
+Implement gammoids using digraphs for data. These will use transversal matroids for taking minors or duals.
It has a working rank function. I think the hard part will be minors from contraction.
Branch pushed to git repo; I updated commit sha1. New commits:
c5cd584 | found the bug |
That should be everything it needs right now.
Branch pushed to git repo; I updated commit sha1. New commits:
0d916eb | Reinstalled sage; documentation builds correctly now |
This looks fairly good, but I noticed a few issues:
this is not merging cleanly with the latest development version, so you'll have to merge and resolve the conflict.
The class documentation might explain "linked into", or point to a reference.
the import of newlabel
does not appear to be needed.
the imports from copy import copy, deepcopy
are not needed either.
The constructor should perhaps check that input D
is in fact a digraph, or can be coerced to one. For example, I might be surprised by
sage: G = graphs.CompleteGraph(3)
sage: g = Gammoid(G, roots=[1]); g
Gammoid of rank 1 on 3 elements
sage: g.digraph().edges()
[(0, 1, None), (0, 2, None), (1, 2, None)]
sage: list(g.bases())
[frozenset({0}), frozenset({1})]
I might have expected the Gammoid derived from graph G to have bases {2} as well. The edge list seems to have arbitrarily directed the edges of G. For another example Gammoid(ZZ, roots=[])
raises a messy attribute error.
_prune_vertices_
remove all irrelevant vertices? And why is it not faster on bad examples? Consider these timings with the existing code:sage: d = digraphs.Circulant(201, [1, 59, 100])
sage: %timeit Gammoid(d+digraphs.Path(1000), roots=[1,50,100,153], groundset=d.vertices())
1 loop, best of 3: 580 ms per loop
sage: %timeit Gammoid(d+digraphs.Path(2000), roots=[1,50,100,153], groundset=d.vertices())
1 loop, best of 3: 1.96 s per loop
sage: %timeit Gammoid(d+digraphs.Path(3000), roots=[1,50,100,153], groundset=d.vertices())
1 loop, best of 3: 4.17 s per loop
Here's one way to be more thorough, and faster in these cases
V = set(self._D.vertices()).difference(self._groundset)
if len(V) == 0:
return
v = self._D.add_vertex()
self._D.add_edges( (v,u) for u in self._groundset)
self._D.add_edges( (u,v) for u in self._roots)
c = self._D.strongly_connected_component_containing_vertex(v)
self._D.delete_vertices(V.difference(c))
self._D.delete_vertex(v)
New timings:
sage: %timeit Gammoid(d+digraphs.Path(1000), roots=[1,50,100,153], groundset=d.vertices())
10 loops, best of 3: 19.3 ms per loop
sage:
sage: %timeit Gammoid(d+digraphs.Path(2000), roots=[1,50,100,153], groundset=d.vertices())
10 loops, best of 3: 39.7 ms per loop
sage: %timeit Gammoid(d+digraphs.Path(3000), roots=[1,50,100,153], groundset=d.vertices())
10 loops, best of 3: 72.4 ms per loop
And here's a case that, while slightly slower, the new code is more accurate in pruning. First, with the original code:
%timeit Gammoid(d+digraphs.Circuit(3000), roots=[1,50,100,153], groundset=d.vertices())
10 loops, best of 3: 49.1 ms per loop
sage: g = Gammoid(d+digraphs.Circuit(3000), roots=[1,50,100,153], groundset=d.vertices())
sage: g.digraph()
Digraph on 3201 vertices
and with the new code
%timeit Gammoid(d+digraphs.Circuit(3000), roots=[1,50,100,153], groundset=d.vertices())
10 loops, best of 3: 70 ms per loop
sage: g = Gammoid(d+digraphs.Circuit(3000), roots=[1,50,100,153], groundset=d.vertices())
sage: g.digraph()
Digraph on 201 vertices
The new code could be better, for example, also deleting roots if they are unreachable from the groundset, and there are of course other ways to code up the pruning.
the "red" colour #D55E00 looks more orange than red, to me.
every minor of a gammoid is a gammoid. Why not have _minor_
recognize that?
_minor_
should have a few more test cases; there are 4 paths through the code and they could all be tested.
In the documentation for gammoid_extension
you refer to the "starting set" and a "source vertex". These are new terms not appearing the documentation of the class or constructor. The documentation for the parameters does better.
The documentation for gammoid extension
could be clearer on what it accomplishes. Is every matroid extension of a gammoid that results in another gammoid obtained in this way?
Implement gammoids using digraphs for data. These will use transversal matroids for taking minors or duals.
CC: @sagetrac-zgershkoff @sagetrac-Stefan @tscrim
Component: matroid theory
Author: Zachary Gershkoff
Branch/Commit: u/zgershkoff/gammoids @
0d916eb
Issue created by migration from https://trac.sagemath.org/ticket/23601