Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#13618 closed enhancement (fixed)

Doctest coverage for rings

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-5.8
Component: doctest coverage Keywords: doctests
Cc: kini Merged in: sage-5.8.beta1
Authors: Travis Scrimshaw Reviewers: Kannappan Sampath, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13634, #12802, #6495 Stopgaps:

Description (last modified by jdemeyer)

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:

Attachments (3)

trac_13618-rings_doc_others-ts.patch (135.8 KB) - added by tscrim 7 years ago.
Everything else
trac_13618-rings_doc_complex-ts.patch (201.3 KB) - added by tscrim 7 years ago.
Rebased
trac_13618-rings_doc_real-ts.patch (312.9 KB) - added by tscrim 7 years ago.
Rebased

Download all attachments as: .zip

Change History (33)

comment:1 Changed 7 years ago by tscrim

  • Dependencies set to #13634

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

comment:2 Changed 7 years ago by tscrim

  • Status changed from new to needs_review

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

comment:3 Changed 7 years ago by tscrim

  • Dependencies changed from #13634 to #13634, #12802
  • Description modified (diff)

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*.

comment:4 Changed 7 years ago by tscrim

Ready for review.

comment:5 Changed 7 years ago by tscrim

  • Description modified (diff)

<edit>Removed patchbot message</edit>

Last edited 7 years ago by tscrim (previous) (diff)

comment:6 follow-up: Changed 7 years ago by vbraun

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

comment:7 in reply to: ↑ 6 Changed 7 years ago by tscrim

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.

comment:8 Changed 7 years ago by vbraun

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)

comment:9 Changed 7 years ago by tscrim

  • Description modified (diff)

Changed. Thank you.

Edit:

For patchbot:

Apply: trac_13618-rings_doc-ts.patch

Last edited 7 years ago by tscrim (previous) (diff)

comment:10 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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

comment:11 Changed 7 years ago by tscrim

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

comment:12 Changed 7 years ago by tscrim

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

comment:13 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Doctest coverage to rings to Doctest coverage for rings

comment:14 Changed 7 years ago by kini

  • Cc kini added

comment:15 Changed 7 years ago by knsam

I hereby intend to review a part of this ticket: the real* bit. I will come up with comments, if any in a few hours. ~KnS

comment:16 Changed 7 years ago by knsam

Oops!! I applied the real* patch but tests fail! sage -t real* spits out a whole bunch of error on Sage 5.7.beta4 (I am on a ubuntu 12.04 64 bit box).

Could you point me the mistake I am making?

comment:17 Changed 7 years ago by tscrim

Hmmm...did the patch apply cleanly? What are the errors?

Also, you should apply the complex* part since that adds a few of the real* files to the doc.

Best,
Travis

comment:18 Changed 7 years ago by tscrim

For the record, this also takes care of #13383.

comment:19 Changed 7 years ago by vbraun

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?

comment:20 Changed 7 years ago by knsam

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

Last edited 7 years ago by knsam (previous) (diff)

Changed 7 years ago by tscrim

Everything else

comment:21 Changed 7 years ago by tscrim

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.

Last edited 7 years ago by tscrim (previous) (diff)

comment:22 Changed 7 years ago by vbraun

  • Reviewers set to Kannappan Sampath, Volker Braun
  • Status changed from needs_review to positive_review

comment:23 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.7 to sage-5.8

comment:24 Changed 7 years ago by jdemeyer

  • Dependencies changed from #13634, #12802 to #13634, #12802, #6495
  • Status changed from positive_review to needs_work

This needs to be rebased to #6495.

Changed 7 years ago by tscrim

Rebased

Changed 7 years ago by tscrim

Rebased

comment:25 Changed 7 years ago by tscrim

  • Status changed from needs_work to positive_review

Rebased.

comment:26 follow-up: Changed 7 years ago by 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.

comment:27 in reply to: ↑ 26 ; follow-up: Changed 7 years ago by tscrim

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?

comment:28 in reply to: ↑ 27 Changed 7 years ago by knsam

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

comment:29 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.8.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:30 Changed 7 years ago by jdemeyer

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.

Note: See TracTickets for help on using tickets.