#19586: Add is_cayley_graph
-------------------------------------+-------------------------------------
Reporter: jaanos | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.10
Component: graph theory | Resolution:
Keywords: Cayley graphs | Merged in:
groups | Reviewers: Nathann Cohen
Authors: Janoš Vidali | Work issues:
Report Upstream: N/A | Commit:
Branch: | 8685628455d2364bbb1896bf28e18c637bfab50c
u/jaanos/add_is_cayley_graph | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by ncohen):
* reviewer: => Nathann Cohen
Comment:
Hellooooooooooo,
Thank you very much for this update. Here are some comments:
- Some people sometimes fight about whether 'self' should be allowed in
the docstrings, .e.g:
{{{Check whether self is a Cayley graph.}}}
I don't insist on any such rule, but I usually write it on the pattern
'Check
whether the graph is a Cayley graph'. As you rightfully added your
method to
the index, you can see that only the *name* of the function appears
before
{{{Check whether self is a Cayley graph.}}}. In this context, the
meaning of
'self' is not clear. This being said, you will probably see it occur in
some
other places of Sage's code: make the change or do not make it,
according to
your taste. I just thought worth mentionning.
- About the doctest {{{sage: S}}}. I am not sure that the output will be
identital on all platforms: the order of the generators can change on a
different architecture, and this would be reported as a broken doctest.
If you
have reasons to believe that it will work on all platform then it can
stay,
otherwise the test must be made platform-independent. This can be done
by
building the graph from the generators for instance. If you do not
believe
that it is worth checking (e.g. if it is already tested elsewhere) then
you
can just remove this line, or test something different.
- `regular_subgroup` given that the default behaviour is to return a
boolean,
shouldn't it be named `has_regular_subgroup` instead?
- about `is_cayley` returning generators: do you think that it is
necessary to
have this option, or should we let users call `.generators()` on the
group
object?
- {{{c, G, d, S}}} could you give more meaningful names to those
variables? I
guess that 'G' is alright (even though, in a Graph function...) but `d`
and
`S` could easily be `graph_to_group_map` and `generators`. Or anything
else
you might prefer.
- You do not need `\` at the end of the line when a `[({` is open and
remains to
be closed.
- Can this
{{{
+ t = C[0].is_cayley(return_group=certificate)
+ if certificate:
+ c, CG = t
}}}
be replaced by that?
{{{
+ c, CG = C[0].is_cayley(return_group=certificate)
+ if certificate:
}}}
The value `None` may be replaced by `None` uselessly, but that's not a
big
problem (if I don't get anything wrong)
- About {{{return tuple(out)}}} -- there is nothing wrong with this,
though I
don't think it is necessary to force a 'tuple'. This is no request to
change
it of course, it is fine as it is if you prefer it this way.
- It is better to call `self.degree()` than `len(self.domain())`.
Nathann
P.S.: Don't forget to set a ticket back to `needs_review` once you are
done with the changes.
--
Ticket URL: <http://trac.sagemath.org/ticket/19586#comment:62>
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 https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
