#14589: binary matrices, dense graphs, and faster is_strongly_regular
-------------------------+-------------------------------------------------
Reporter: | Owner: jason, ncohen, rlm
ncohen | Status: needs_review
Type: | Milestone: sage-6.1
enhancement | Resolution:
Priority: major | Merged in:
Component: graph | Reviewers:
theory | Work issues:
Keywords: | Commit:
Authors: | b56444ba216f3c2e6d7f20e34daf80f5d5aec781
Nathann Cohen | Stopgaps:
Report Upstream: N/A |
Branch: |
u/ncohen/14589 |
Dependencies: |
#14805 |
-------------------------+-------------------------------------------------
Comment (by azi):
Replying to [comment:15 ncohen]:
> Yooooooo !
>
> > As promised I took the time to review your patch.
>
> Thaaaaaaaaaaanks !! `;-)`
>
> > The entire thing is easy to read and looks good. The fact that it was
not reviewed for so long makes me wonder if I am overlooking some non
obvious caveat?
>
> The "someone else with do it" syndrome. And the fact that with git the
developpers as a whole probably suffered a loss of enthusiasm `:-P`
>
> > 1. Line 67 - can we always assume that g.vertices() will return a
sorted list of vertices? Also, it looks like this can be written in a
better way?
>
> Unfortunately I think we can. Though I try not to rely on that. Did I do
it in this patch ?
Yes didn't you?
>
> The problem is that while the vertices are indeed "sorted" before being
returned, but that this sorting can be meaningless : the "natural"
ordering on the vertices may be a partial ordering from time to time (i.e.
when the vertices are set : `graphs.KneserGraph`) and well, that causes
bugs from time to time.
>
> > 2. Line 48 function inline binary_matrix_free. Shouldn't you free
m.rows[1],...,m.rows[n_rows] as well?
>
> No, because only `m.rows[0]` is allocated on line 39. All of `m.rows[i]`
point toward some part of this very long vector, but regardless of that if
you deallocate `m.rows[0]` the whole segment is deallocated.
Right! I completely misread something somewhere.
>
> > 3. Line 69 in inline binary_matrix_set0 the comment should really say
set matrix entry to 0
>
> Oh. Right `:-PPPPPPPPP`
>
> > Also off topic to this patch - am I the only one being tempted to
change the function edges() and set edge_labels to false as default?
>
> `T_T`
>
> I hate labels. And I also type `.edges(labels=False)` much more often
than `.edges()`. Well. I didn't try to change this kind of stuff because I
consider that it is just "my own use of graphs"... But well, if many of us
begin to hate labels/loops/multiple edges now that's a different story
`:-P`
> The combinat/word guys may probably not agree, though... But really, I
have no idea. I just never did that because I don't want to change stuff
just because it makes it easier for me, without knowing how useful it can
be for others.
For me they are veeeery annoying so I assumed they were for everyone .
Anyways, this is all I got to say about the patch. It looks good to me
provided u changed the above things and ran the doctests (which I did not)
Best,
Jernej
>
> Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/14589#comment:20>
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/groups/opt_out.
