#15491: directed immutable graphs report twice too many edges
-------------------------+-------------------------------------------------
Reporter: | Owner:
ncohen | Status: needs_review
Type: | Milestone: sage-5.13
defect | Resolution:
Priority: major | Merged in:
Component: graph | Reviewers:
theory | Work issues:
Keywords: | Commit:
Authors: | cd9792698e90de72e0980a315a7e76db6e38d676
Nathann Cohen | Stopgaps:
Report Upstream: N/A |
Branch: |
u/ncohen/15491 |
Dependencies: |
-------------------------+-------------------------------------------------
Comment (by ncohen):
Yo !
> For a review, I need some information on `num_edges(self,directed)`.
Okay. Just to make things clear, I don't like this function, it was just
part of the `sparse_graph` backend and so I implemented the very same for
this new backend.
> - In the doc string, you start to tell us something:
I removed the `)`.
> - You define `cdef unsigned int m` but don't use it. I guess this should
be removed.
Done.
> - In the case of an undirected graph whose number of directed edges are
requested, you do `return int(cg.g.neighbors[cg.g.n]-cg.g.edges)`. Can you
please explain why this is equal to twice the number of edges minus the
number of loops?
`cg.g.edges` is not equal to the number of edges. It is a pointer toward
the beginning of the `edges` array. `cg.g.edges` is actually equal to
`cg.g.neighbors[0]`. I added a line of doc to emphasize it. I'm
substracting pointers there, not integers.
> Also there is trailing whitespace in error messages:
> {{{
> raise NotImplementedError("Sorry, I have no idea what is
expected "
> "in this situation. I don't
think "
> "that it is well-defined
either, "
> "especially for multigraphs.")
> }}}
Which trailing whitespace ? `O_o`
You mean the spaces just before the `"` ? If I remove them you will see
"expectedin" and "thinkthat" when the message is displayed. I added a
doctest to make this clear.
> And I think the French rules of typography shouldn't be used in an
English text. Hence, replace `bla : bla` by `bla: bla`. I guess this will
be part of a review commit.
There is no occurrence of " : " in this file, and to be honest I could not
care less about the spaces between and after the ":". I am french, and
most of the books I read are english. I don't care whether there is a " :
" or ": ", I don't even notice the difference. My problem with spaces
before/after ":" is that there are grammar nazis on both sides : the
english complain when I put spaces, the french when I don't. Honestly do
whatever you want with that. Though there again, I couldn't find any
occurrence of " : " in that file.
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/15491#comment:5>
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.
