#13192: some code clean up for sage/graphs/graph.py
----------------------------------+-----------------------------------------
Reporter: eisermbi | Owner: jason, ncohen, rlm
Type: defect | Status: needs_review
Priority: major | Milestone: sage-5.2
Component: graph theory | Resolution:
Keywords: sparse6_string | Work issues:
Report Upstream: N/A | Reviewers: Nathann Cohen, Karl-Dieter
Crisman
Authors: Birk Eisermann | Merged in:
Dependencies: #13109 | Stopgaps:
----------------------------------+-----------------------------------------
Changes (by eisermbi):
* status: needs_work => needs_review
Comment:
Replying to [comment:13 kcrisman]:
> Your update apparently added a lot of whitespace changes again.
My last update did ''not'' touch the whitespaces. In a previous update I
reduced the number of changed lines from 10 to 6 lines. These trailing
whitespace changes are only within the documentation of the method
`sparse6_string`, which does not seem to be modified for years(?,
...according to my search on sage-trac). It reads like removing trailing
whitespace is forbidden. Hence, I have moved all these changes into a
separate file and you are at liberty to exclude or include them - no
question asked! ;-)
> Also, what was the doctest error? Did you change the code at all?
Am I lying? - joke - Okay, the change was small! It added exactly the line
on that you were commenting. When (restoring and) deprecating the old
function I did not think about that the deprecation will affect all
doctests where this functions is called. (In my case luckily only one
place - the doctest of the deprecated function.)
> I think generally in doctests we go with
> {{{
> doctest:...: DeprecationWarning: compare
> }}}
> instead of
> {{{
> doctest:1: DeprecationWarning: compare
> }}}
> in case things were to go in a different order. At any rate, that's
what I've seen.
>
Unfortunately, my first discovery was "doctest:1:" (and there are a few
such places in the source code) and I copied that. - Now it is changed.
> Also, even if you aren't removing the function, presumably it was still
testing something useful. Would it be possible to add a graph like the
kind Nathann suggests (with non-integer vertices) that shows that this new
code is properly comparing, as well as showing it properly compares the
edges tested before? I agree that it's good (or at least I said I did
above!), just that we want to have documentation.
The change was about sorting graph edges. Thus, I added some doctests that
sort the edges once by the old method and once by the new method and
compare the results. As suggested I put these doctests in the deprecated
function.
> Basically, what you could do is move the tests in the old function to
the tests for the actual meaningful function, and then leave only a very
minimal doctest in the old function that states that this is deprecated in
the docstring, and has ''one'' test exactly to test the deprecation
warning.
I reduced the old doctests to just one. It will save a few millionths of a
second for doctesting ;-) Well, in this case there was not much doctesting
to remove...
Birk
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13192#comment:14>
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 post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
