#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 SimonKing):
For a review, I need some information on `num_edges(self,directed)`.
- In the doc string, you start to tell us something:
{{{
INPUT:
- ``directed`` (boolean) -- whether to consider the graph as
directed or
not (
}}}
Should there be some explanation after the opening bracket, or should
the opening bracket be removed?
- You define `cdef unsigned int m` but don't use it. I guess this should
be removed.
- 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? In `static_sparse_graph.pyx` I read
{{{
* ``neighbors`` (``unsigned short **``) -- this array has size `n+1`,
and
describes how the data of ``edges`` should be read : the neighbors
of
vertex `i` are the elements of ``edges`` addressed by
``neighbors[i]...neighbors[i+1]-1``. The element ``neighbors[n]``,
which
corresponds to no vertex (they are numbered from `0` to `n-1`) is
present
so that it remains easy to enumerate the neighbors of vertex `n-1` :
the
last of them is the element addressed by ``neighbors[n]-1``.
}}}
By combining both comments, I guess that `neighbors[n]` gives three
times the number of edges minus the number of loops (so that
`cg.g.neighbors[cg.g.n]-cg.g.edges` is two times the number of edges minus
the number of loops, as requested), and thus the last neighbour of vertex
`n-1` is three times the number of edges minus the number of loops minus
one---but if that's the case then it should be stated somewhere.
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.")
}}}
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.
--
Ticket URL: <http://trac.sagemath.org/ticket/15491#comment:3>
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.
