#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 )
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)
Change History (33)
comment:1 Changed 7 years ago by
- Dependencies set to #13634
comment:2 Changed 7 years ago by
- 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 6 years ago by
- 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 6 years ago by
Ready for review.
comment:6 follow-up: ↓ 7 Changed 6 years ago by
The doctests fail, you need separate # 32bit / # 64bit cases for the __hash__
'es
comment:7 in reply to: ↑ 6 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
- Description modified (diff)
Changed. Thank you.
Edit:
For patchbot:
Apply: trac_13618-rings_doc-ts.patch
comment:10 Changed 6 years ago by
- 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 6 years ago by
I'll upgrade my version of sage tonight and rebase it.
comment:12 Changed 6 years ago by
- 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 6 years ago by
- Description modified (diff)
- Summary changed from Doctest coverage to rings to Doctest coverage for rings
comment:14 Changed 6 years ago by
- Cc kini added
comment:15 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
For the record, this also takes care of #13383.
comment:19 Changed 6 years ago by
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 6 years ago by
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:
- In
real_double
,real_mpfr
: It might be nice if we codify the PARI commands too. For instance, the algorithm foralgebraic_dependence()
andalgdep()
, we refer to the PARI commandalgdep
. - In
real_mpfi
: In the documentation for the methodstr()
, could you please use\cdot
instead of*
? - 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 forupper()
is not formatted properly. (I'd be totally OK if this goes into another ticket.)
Else, as I said, this is fantastic!
~KnS
comment:21 Changed 6 years ago by
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.
comment:22 Changed 6 years ago by
- Reviewers set to Kannappan Sampath, Volker Braun
- Status changed from needs_review to positive_review
comment:23 Changed 6 years ago by
- Milestone changed from sage-5.7 to sage-5.8
comment:24 Changed 6 years ago by
- Dependencies changed from #13634, #12802 to #13634, #12802, #6495
- Status changed from positive_review to needs_work
This needs to be rebased to #6495.
comment:26 follow-up: ↓ 27 Changed 6 years ago by
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: ↓ 28 Changed 6 years ago by
comment:28 in reply to: ↑ 27 Changed 6 years ago by
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 6 years ago by
- Merged in set to sage-5.8.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
comment:30 Changed 6 years ago by
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.
The dependency on #13634 is for the output of scientific notation of complex interval field.