#19136: NO and NU graphs
-------------------------+-------------------------------------------------
Reporter: | Owner:
dimpase | Status: needs_work
Type: | Milestone: sage-6.9
enhancement | Resolution:
Priority: major | Merged in:
Component: graph | Reviewers: Nathann Cohen
theory | Work issues:
Keywords: | Commit:
Authors: | 2bd5f54053f39da78f3c610ee009f6f8bf89a440
Report Upstream: N/A | Stopgaps:
Branch: |
u/dimpase/NONU |
Dependencies: |
#19098, #19180 |
-------------------------+-------------------------------------------------
Changes (by ncohen):
* status: needs_review => needs_work
* reviewer: => Nathann Cohen
Comment:
Hello Dima,
Here is a first-pass review.
- `graphs.SymplecticGraph` does not exist. It is not enough to create a
deprecation alias, it must also be available in `graphs.<tab>`.
- In `NonisotropicOrthogonalPolarGraph`. Replace
{{{
else:
if not perp is None:
<code>
else:
}}}
with
{{{
elif not perp is None:
<code>
else:
}}}
No need to indent everything twice.
- same function: `q` does not appear in the INPUT block
- same function: move the checks on q (prime power) and sign (+-) to the
beginning of the function (not in a 'if' block). This function builds a
(dense) graph, we are not at a level where we try to minimize the amount
of
unnecessary 'if'.
- REFERENCE block happens at the end of the docstring
- Remove the commented code that your functions contain
- Why 5 `is_NO*` functions instead of one `is_NO` ? I remember a comment
of
yours about copy/paste...
- If your function is slow it is because of that line
{{{
G = Graph([V,lambda x,y:P(vector(x),vector(y))==0],loops=False)
}}}
When you do this, 'vector' is called 'binomial(n,2)' times, and `vector`
is
*SLOW*. In a previous branch of yours, I added a commit to remove this
in
order to gain a large speedup (do you remember)? Try to find a way to
avoid
vectors. If you cannot, then instead of building `n^2` vectors you
should turn
everything into a vector *once*, and work on vectors instead. Observe
that
vectors have a `.set_immutable` method.
Then, perhaps we will be able to remove those `# long time` everywhere
in
`_orthogonal_polar_graph` (and elsewhere).
- Same in `UnitaryPolarGraph`.
Next time, please think of the reviewer when you built your commits. When
you
save minutes by not doing it, the reviewer loses hours trying to figure
out what
exactly you did.
Thanks for the addition, however. I couldn't have done that.
Nathann
P.S.: Fill the 'authors' field. Think of doing it when you set your
tickets to `needs_review`.
--
Ticket URL: <http://trac.sagemath.org/ticket/19136#comment:23>
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.
