#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 | 6bc5cbd678d6d9db6e99ff23489e0e1a720e8569
Dependencies: #18960, #18948, | Stopgaps:
#18988, #18991, #18986, #19018, |
#19019 |
-------------------------------------+-------------------------------------
Comment (by ncohen):
Helloooooooo Dima,
Here is another review. I also fixed a couple of things in a commit at
`public/18972`.
- About `designs.is_twograph` -- this has nothing to do in the design
catalog.
- About `designs.TwoGraph` -- I believe that it is the same: a class
constructor
does not have anything to do in the design catalog. You don't find
`PermutationGroup` in `groups.<tab>`, and neither do you find `Graph` in
`graphs.<tab>`. Note that there is an 'exception' in this case,
i.e. `BlockDesign`, which (I believe) should also be removed, and for
the same
reasons.
- The first letter of the docstrings should be in upper case. I told you
that
already.
- There are still broken links. If you can't be bothered to read the doc,
at
least compile it with `--warn-links`.
- No need of indentation after a 'INPUT' block.
- Backticks are required around `True/False`.
- option `check` of `is_regular` -- checking that the input is indeed a
twograph
should be done in `TwoGraph.__init__`.
- Instead of `Return True if [...]`, it would be better to have `Test
whether
[...]`. Formally, a function that *always* returns `True` satisfies your
description (I know, that's a stupid thing to say. Still).
- `complement` -- the default meaning of `.complement()` is different on
`IncidenceStructure` and on `TwoGraph`. Could you conform to the most
general
specification?
- `T.is_t_design(k=3)` -- use `is_uniform`.
- It feels a bit weird that the default behaviour of `seidel_switching` is
not
to modify the graph inplace but to return a copy... I would have
expected a
`inplace=False` argument for that.
- Documentation of `seidel_switching`: some mathematical notations need
backticks.
- Same function: the OUTPUT section is hardly necessary, given the first
two
lines of doc.
- `twograph` -- I am tired of reminding you to write doctests.
- Code of `twograph` -- please rewrite the code of this function decently.
- `is_two_graph_descendant_of_srg` -- you seem to have prefered 'twograph'
everywhere else.
- Please build and read the doc of that function. Also, `lambda` should be
`\lambda`.
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/18972#comment:81>
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.
