#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 |
-------------------------------------+-------------------------------------
Comment (by dimpase):
Replying to [comment:52 ncohen]:
>
> Here is the review you requested:
Thanks!
>
> - `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.
OK, sure.
>
> - 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).
this is an urban myth that the latter is much faster:
{{{
sage: ll=range(50000)
sage: timeit('[x for x in ll if x%2==0]')
25 loops, best of 3: 19.8 ms per loop
sage: timeit('filter(lambda x: x%2==0, ll)')
25 loops, best of 3: 22.2 ms per loop
sage: timeit('[x**2 for x in ll]')
25 loops, best of 3: 19.7 ms per loop
sage: timeit('map(lambda x: x**2, ll)')
25 loops, best of 3: 23.7 ms per loop
}}}
besides, I do find the list comprehension syntax ugly and error-prone. All
these
`for ah if that or what not` is very hard to parse for me. Yes, I wrote a
bit of lisp code in my previous life, and my brain is damaged by
parentheses. :-)
>
> - `Graph([V, lambda i, j: frozenset((i,j)) in edges])` THINK ABOUT WHAT
IT
> DOES. This is insulting.
Indeed, I wish I could write `Graph(V,edges)` or `Graph([V,edges])` or
even `Graph(edges)` instead. Why can't I? Why must I instead do something
like
`blah=Graph(V); blah.add_edges(edges); return blah`? Cause the GC is
hungry and must be fed `blah` on each return?
>
> - `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.
I copied this one from the ` elif format == 'adjacency_matrix':` block.
Should one fix both places?
>
> - 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
OK, will do.
>
> - Stop using `\` at the end of lines. If you need to span an instruction
over
> several lines, it is more readable to use `()`,
not to me, sorry. I tend to (mis)read `a=(1+2)` as `a=(1+2,)`.
> 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.
the docs to `g.copy()` say something like "use only if you change the
backend..."
{{{
Warning: Please use this method only if you need to copy but
change the underlying implementation or weightedness. Otherwise
simply do "copy(g)" instead of "g.copy()".
}}}
Should I ignore this? Should the docs be improved?
>
> - Really, STOP using lambda functions. You don't know what it does, so
stop
> that.
This is a myth that they are slow, see above.
> {{{
> + 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.
in my book using temporary variables is slow; perhaps Python
implementations will get fuller use of this fact as its devs and users get
wiser...
>
> - `is_two_graph_descendant_of_srg` -- could you remove those '0'
everywhere in
> the variables?
well, I have v and v0, l and l0 - they are sets of parameters of different
graphs...
Or I **had**, and then optimised v, l, mu away, sorry. I'll see what I can
do.
>
> - `is_even`.
I like it better than `%`, more readable: one does not need to swap out
some other language in the head to recall the meaning. But if you
insist...
--
Ticket URL: <http://trac.sagemath.org/ticket/18972#comment:53>
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.
