#18972: twographs and Seidel switching
-------------------------------------+-------------------------------------
Reporter: dimpase | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.9
Component: graph theory | Resolution:
Keywords: | Merged in:
Authors: | Reviewers: Nathann Cohen
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/dimpase/seidelsw | 1b7f6b99019699fdccb4bb9afa86315defc87994
Dependencies: #18960, #18948, | Stopgaps:
#18988, #18991 |
-------------------------------------+-------------------------------------
Changes (by ncohen):
* status: needs_review => needs_work
* reviewer: => Nathann Cohen
Comment:
Helloooooooooooooo,
Here is the review you requested:
- `TwoGraph` -- Probably should not be in the global namespace.
- Class documentation: there is none. Write one, and redirect toward the
module's doc if necessary for the longer explanations.
- `is_regular` -- clashes with standard hypergraph terminology. Should be
renamed (see #18986).
- `is_regular` -- parameters, role and definition not documented.
- Stop using map and filter. You do not know what it does, i.e. Python is
not
lisp. You are building in memory lists that you do not need, only to
throw
them away later. Use list-comprehension instead (also faster).
- `Graph([V, lambda i, j: frozenset((i,j)) in edges])` THINK ABOUT WHAT IT
DOES. This is insulting.
- `complement` -- Rely on #18986.
- What the hell is this 'functions' in the middle of the file? Does it
produce
anything in the html doc?
- {{{#. A Sage Seidel adjacency matrix}}} -- link toward the doc of
seidel
adjacency matrix, in case people might want to read a definition.
- {{{M[i,j]=-1 indicating that (i,j) is an edge, otherwise M[i,j]=1.}}}
missing
backticks.
- {{{data = data.change_ring(ZZ)}}} -- you don't copy a matrix just
because you
expect `Integer(1)` and get `float(1)`. Test that the entries belong to
a set
you like instead.
- Repeated sentence
{{{
+ Returns the Seidel adjacency matrix of self.
+
+ Returns the Seidel adjacency matrix of the graph.
}}}
- The sentence that follows the repeated sentence is not very clear. Use a
list
to define the objects, it should make it clearer.
- `change_ring` -- should be a sphinx link
- Stop using `\` at the end of lines. If you need to span an instruction
over
several lines, it is more readable to use `()`, e.g.
{{{
a = (1 +
2 +
3 )
}}}
- `seidel_switching` -- repeated sentence, as previously.
- Backticks are missing in the text again.
- Use `g.copy()` (which should be called by `copy(g)`), not deepcopy.
- Really, STOP using lambda functions. You don't know what it does, so
stop
that.
{{{
+ return Graph([Nv+NonNv, lambda i, j:
+ (i in NonNv and j in NonNv and i in
self.neighbors(j)) or
+ (i in Nv and j in Nv and i in
self.neighbors(j)) or
+ (i in Nv and j in NonNv and not i in
self.neighbors(j)) or
+ (j in Nv and i in NonNv and not i in
self.neighbors(j))])
}}}
Build this graph by adding edges to it. It will be incomparably
faster. Writing code like this should not be allowed.
- `is_two_graph_descendant_of_srg` -- could you remove those '0'
everywhere in
the variables?
- `is_even`.
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/18972#comment:52>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
