Doctest coverage for rings

[tscrim]

Adding doctests to

• rings/complex*
• rings/homset.py
• rings/ideal.py
• rings/rational*
• rings/real*

as part of #12024 and cleaning up the documentation.

---

Apply:

• attachment: trac_13618-rings_doc_real-ts.patch
• attachment: trac_13618-rings_doc_complex-ts.patch
• attachment: trac_13618-rings_doc_others-ts.patch

Depends on #13634
Depends on #12802
Depends on #6495

CC: @kini

Component: doctest coverage

Author: Travis Scrimshaw

Reviewer: Kannappan Sampath, Volker Braun

Merged: sage-5.8.beta1

Issue created by migration from https://trac.sagemath.org/ticket/13618

[tscrim]

comment:1

The dependency on #13634 is for the output of scientific notation of complex interval field.

[tscrim]

Dependencies: #13634

[tscrim]

comment:2

If someone is willing to review this, please also review the smaller #13634. Thank you.

[tscrim]

Changed dependencies from #13634 to #13634, #12802

[tscrim]

comment:3

Adding #12802 as a dependency since it modifies ideal.py. I will upload a rebased patch in the near future once I finish rings/real*.

[tscrim]

comment:4

Ready for review.

[tscrim]

comment:5

Removed patchbot message

[vbraun]

comment:6

The doctests fail, you need separate # 32bit / # 64bit cases for the __hash__'es

[tscrim]

comment:7

Replying to @vbraun:

The doctests fail, you need separate # 32bit / # 64bit cases for the __hash__'es

What's the best way to do so? Thanks.

[vbraun]

comment:8

Here is an example from sage/rings/polynomial/infinite_polynomial_element.py

    def _hash_(self):
        """
        TESTS::

            sage: X.<x> = InfinitePolynomialRing(QQ)
            sage: a = x[0] + x[1]
            sage: hash(a) # indirect doctest
            -6172640511012239345   # 64-bit
            -957478897             # 32-bit

        """
        return hash(self._p)

[tscrim]

comment:9

Changed. Thank you.

Edit:

For patchbot:

Apply: trac_13618-rings_doc-ts.patch

[jdemeyer]

comment:10

It would be a pity to lose all this work, any chance it can be rebased and fixed?

[tscrim]

comment:11

I'll upgrade my version of sage tonight and rebase it.

[tscrim]

comment:12

I've rebased and split it into 3 more manageable files.

For patchbot:

Apply: attachment: trac_13618-rings_doc_real-ts.patch, trac_13618-rings_doc_complex-ts.patch, trac_13618-rings_doc_others-ts.patch

[vbraun]

comment:19

I've looked through the other two patches and they look good to me except that there is lots of unnecessary whitespace change. This does nothing but break other patches on trac. We'll programmatically remove trailing whitespace when we change to git. Can you go through them and at least remove the patch hunks that do nothing but whitespace change? Like this

[...]
@@ -359,9 +422,9 @@ class Ideal_generic(MonoidElement):
     def base_ring(self):
         r"""
         Returns the base ring of this ideal.
-        
+
         EXAMPLES::
-        
+
             sage: R = ZZ
             sage: I = 3*R; I
             Principal ideal (3) of Integer Ring
@@ -370,16 +433,16 @@ class Ideal_generic(MonoidElement):
[...]

Kannappan, are you still reviewing the "real" patch?

[KPanComputes]

comment:20

Oops! Sorry for the delay!! I had to look through, because reviewing doctests is new to me. But, I have just finished it!

Here are some trivial comments, rectifying these as necessary should give this positive review:

1. In real_double, real_mpfr: It might be nice if we codify the PARI commands too. For instance, the algorithm for algebraic_dependence() and algdep(), we refer to the PARI command algdep.
2. In real_mpfi: In the documentation for the method str(), could you please use \cdot instead of *?
3. In real_interval_absolute: Some docstrings are too short to be useful; we'd also would have to look into the grammar and codify appropriate things like self. The example for upper() is not formatted properly. (I'd be totally OK if this goes into another ticket.)

Else, as I said, this is fantastic!

~KnS

[tscrim]

Attachment: trac_13618-rings_doc_others-ts.patch.gz

Everything else

[tscrim]

comment:21

Alright, I've removed all the whitespace hunks I could find and addressed all of Kannappan's comments as much as I could. Thank you both for reviewing this!

Thank you,

Travis

PS - Also incase anyone is feeling extra generous (or wants me to owe them another favor) there's also the followup #13685.

[vbraun]

Reviewer: Kannappan Sampath, Volker Braun

[jdemeyer]

Changed dependencies from #13634, #12802 to #13634, #12802, #6495

[jdemeyer]

comment:24

This needs to be rebased to #6495.

[tscrim]

Attachment: trac_13618-rings_doc_complex-ts.patch.gz

Rebased

[tscrim]

Rebased

[tscrim]

comment:25

Attachment: trac_13618-rings_doc_real-ts.patch.gz

Rebased.

[jdemeyer]

comment:26

Congratulations!

Overall weighted coverage score:  90.6%
Total number of functions:  31495
We need 1397 more functions to get to 95% coverage.
We need 2657 more functions to get to 99% coverage.

[tscrim]

comment:27

Replying to @jdemeyer:

Congratulations!

Overall weighted coverage score:  90.6%
Total number of functions:  31495
We need 1397 more functions to get to 95% coverage.
We need 2657 more functions to get to 99% coverage.

Yay! So what should we do about #12024? Set it to positive review?

[KPanComputes]

comment:28

Replying to @tscrim:

Yay! So what should we do about #12024? Set it to positive review?

IMHO, that could still serve as a meta ticket, so that all we can track all the work proposed to be undertaken by various developers and those unclaimed still remain. Perhaps, we should change the title to something else. But, not close it. Just my feeling, but feel free to ignore. ~KnS

[jdemeyer]

Merged: sage-5.8.beta1

[jdemeyer]

comment:30

Since this patch is already merged, the following advise can only serve as a warning for the future: you should not make needless whitespace changes. Changing whitespace all over the place makes rebasing more difficult than it should be.