Ticket #2507 (assigned defect)

Opened 10 months ago

Last modified 3 months ago

[with patch; needs work] Docstrings and Doctests for rings/quotient_ring_element.py

Reported by: cswiercz Assigned to: cswiercz (accepted)
Priority: minor Milestone: sage-3.4
Component: doctest Keywords: docstring, doctest
Cc:

Description

Current coverage in Sage 2.10.3: SCORE quotient_ring_element.py: 3% (1 of 27)

Attachments

rings.quotient_ring_element.patch (11.6 kB) - added by cswiercz on 04/02/2008 02:11:12 PM.
doctests and docstrings [SCORE quotient_ring_element.py: 37% (10 of 27)]
2507.patch (11.8 kB) - added by mhansen on 04/07/2008 04:00:38 PM.
2507_additional.patch (3.6 kB) - added by cswiercz on 04/17/2008 03:23:59 PM.
Addresses (sort of) the issues brought up by malb. Will additional detailed examples in a later patch. APPLY AFTER 2507.patch.

Change History

03/13/2008 08:59:57 PM changed by cswiercz

  • owner changed from failure to cswiercz.

04/02/2008 02:11:12 PM changed by cswiercz

  • attachment rings.quotient_ring_element.patch added.

doctests and docstrings [SCORE quotient_ring_element.py: 37% (10 of 27)]

04/07/2008 03:54:34 PM changed by mabshoff

  • summary changed from Docstrings and Doctests for rings/quotient_ring_element.py to [with patch, needs reivew] Docstrings and Doctests for rings/quotient_ring_element.py.

Chris,

please make sure that you properly mark tickets with doctests added. Otherwise we cannot find them since attaching patches does not trigger an email to sage-trac.

Cheers,

Michael

04/07/2008 04:00:38 PM changed by mhansen

  • attachment 2507.patch added.

04/07/2008 04:01:13 PM changed by mhansen

  • summary changed from [with patch, needs reivew] Docstrings and Doctests for rings/quotient_ring_element.py to [with patch, positive reivew] Docstrings and Doctests for rings/quotient_ring_element.py.

2507.patch applies against 3.0.alpha2 and passes tests.

04/07/2008 04:32:54 PM changed by mabshoff

This patch needs to be rebase. It fails to apply cleanly against my current merge tree: 

sage-3.0.alpha3/devel/sage$ patch -p1 < trac_2507_rings.quotient_ring_element.patch
patching file sage/rings/quotient_ring_element.py
Hunk #1 FAILED at 66.
Hunk #2 succeeded at 147 (offset 2 lines).
Hunk #3 succeeded at 274 (offset 2 lines).

Cheers,

Michael

04/10/2008 04:23:09 PM changed by malb

  • summary changed from [with patch, positive reivew] Docstrings and Doctests for rings/quotient_ring_element.py to [with patch, mixed review] Docstrings and Doctests for rings/quotient_ring_element.py.

I really like that someone sat down to write doctests for quotient_ring_element.py, but I see some issues: 

sage: Q = ZZ.quotient(10*ZZ)
sage: type(Q)
<class 'sage.rings.integer_mod_ring.IntegerModRing_generic'>
sage: type(Q.gen())
<type 'sage.rings.integer_mod.IntegerMod_int'>

and 

sage: R.<a> = QuotientRing(QQ[x], x^2 + 1)
sage: type(R)
<class 'sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_field'>
sage: type(a)
<class 'sage.rings.polynomial.polynomial_quotient_ring_element.PolynomialQuotientRingElement'>

Thus, the documentation doesn't actually illustrate the class. AFAIK this class is mainly useful/used for multivariate polynomial rings. This theory reinforced by the lm() and monomials() methods. This class is just a mess, maybe?

04/17/2008 03:23:59 PM changed by cswiercz

  • attachment 2507_additional.patch added.

Addresses (sort of) the issues brought up by malb. Will additional detailed examples in a later patch. APPLY AFTER 2507.patch.

04/17/2008 04:53:47 PM changed by cswiercz

  • status changed from new to assigned.
  • summary changed from [with patch, mixed review] Docstrings and Doctests for rings/quotient_ring_element.py to [with patch; needs review] Docstrings and Doctests for rings/quotient_ring_element.py.

04/18/2008 01:06:16 AM changed by malb

I am not sure this is the right fix: Doctests should test and demonstrate the class and it doesn't make sense to create the QuotientRing?(ZZ,10) when ZZ.quotient(10) is available and much faster. Why not test it with the rings it is intended for (multivariate polynomials)?

06/03/2008 07:36:01 AM changed by malb

  • summary changed from [with patch; needs review] Docstrings and Doctests for rings/quotient_ring_element.py to [with patch; mixed review, might need work] Docstrings and Doctests for rings/quotient_ring_element.py.

09/26/2008 12:24:26 PM changed by mabshoff

  • summary changed from [with patch; mixed review, might need work] Docstrings and Doctests for rings/quotient_ring_element.py to [with patch; needs review] Docstrings and Doctests for rings/quotient_ring_element.py.

Mmm, this patch might have bitrotted. Anyway, it still needs a review.

Cheers,

Michael

09/28/2008 07:18:51 AM changed by malb

  • summary changed from [with patch; needs review] Docstrings and Doctests for rings/quotient_ring_element.py to [with patch; needs work] Docstrings and Doctests for rings/quotient_ring_element.py.

I think this patch needs work since it doesn't doctest the main purpose of this class: multivariate polynomial quotients.