Opened 11 years ago

Closed 11 years ago

#9443 closed defect (fixed)

infinite polynomial ring is_integral_domain and is_field omit optional argument 'proof'

Reported by: niles Owned by: AlexGhitza
Priority: major Milestone: sage-4.6
Component: algebra Keywords: infinite polynomial ring
Cc: Merged in: sage-4.6.alpha1
Authors: Niles Johnson Reviewers: Simon King
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by niles)

Other implementations of is_integral_domain allow an argument 'proof' whose default value is False. Infinite polynomial ring omits this argument in its definition of is_integral_domain:

sage: W = PolynomialRing(InfinitePolynomialRing(QQ,'a'),2,'x,y')
sage: W.is_integral_domain()
-------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
TypeError: is_integral_domain() takes exactly 1 argument (2 given)

same goes for is_field:

sage: W = PowerSeriesRing(InfinitePolynomialRing(QQ,'a'),'x')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
TypeError: is_field() got an unexpected keyword argument 'proof'

Attachments (5)

trac_9943_default_arguments.patch (1.2 KB) - added by niles 11 years ago.
add argument 'proof' with default value False to is_field and is_integral_domain
trac_9943_default_arguments_doctests.patch (3.0 KB) - added by niles 11 years ago.
apply after previous patch; includes doctests, and updates a few functions to accept positional and keyword arguments; removes duplicate definition of is_field
trac_9443_default_arguments_combined.patch (3.1 KB) - added by niles 11 years ago.
compined patch replacing the previous two; patch name and commit message fixed
trac_9443_default_arguments_combined_rebased.patch (3.0 KB) - added by niles 11 years ago.
rebased against #9114 (for 4.5.2); apply only this patch.
trac_9443_default_arguments_combined_rebased.2.patch (3.2 KB) - added by mpatel 11 years ago.
Restore commit string. Apply only this patch.

Download all attachments as: .zip

Change History (17)

Changed 11 years ago by niles

add argument 'proof' with default value False to is_field and is_integral_domain

comment:1 Changed 11 years ago by niles

  • Description modified (diff)

comment:2 Changed 11 years ago by niles

  • Description modified (diff)

comment:3 Changed 11 years ago by niles

  • Status changed from new to needs_review

Changed 11 years ago by niles

apply after previous patch; includes doctests, and updates a few functions to accept positional and keyword arguments; removes duplicate definition of is_field

comment:4 follow-up: Changed 11 years ago by SimonKing

  • Priority changed from trivial to major
  • Status changed from needs_review to needs_work
  • Work issues set to Add ticket number to commit message

Thank you for working on Infinite Polynomial Rings! Why didn't you add me (as original author) to Cc? I think I am a natural candidate for being reviewer...

First of all, the patches apply cleanly, and sage -b does not complain.

Second, I think the patches provide a clean solution. I am sorry that I didn't use *args and **kwds in the first place.

Third, it is a formal requirement that the commit message of each patch must point to the relevant ticket. So, could you please add "#9443: " or so to the commit messages? Moreover, the attachments name a wrong ticket number (9943 rather than 9443).

Fourth, I am now running make ptestlong and report back whether it has worked.

Fifth, since you fix a bug, I believe the priority is certainly not "trivial". I am promoting it to "major".

comment:5 Changed 11 years ago by SimonKing

make ptestlong is not done yet. But at least I can confirm that the original problem is fixed:

sage: W = PolynomialRing(InfinitePolynomialRing(QQ,'a'),2,'x,y')
sage: W.is_integral_domain()
True
sage: W = PowerSeriesRing(InfinitePolynomialRing(QQ,'a'),'x')
sage: W
Power Series Ring in x over Infinite polynomial ring in a over Rational Field

comment:6 in reply to: ↑ 4 ; follow-up: Changed 11 years ago by SimonKing

  • Reviewers set to Simon King
  • Status changed from needs_work to positive_review

All tests pass.

So, I can give this a positive review - modulo the minor nitpicking about the commit message. This is easy to change.

I hope it is, in this case, correct to mark this ticket as "positive review" but keep the "work issues" field.

Changed 11 years ago by niles

compined patch replacing the previous two; patch name and commit message fixed

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

Thanks! The combined patch should now be complete.

comment:8 Changed 11 years ago by niles

  • Work issues Add ticket number to commit message deleted

comment:9 follow-up: Changed 11 years ago by mpatel

  • Status changed from positive_review to needs_work

Applying trac_9443_default_arguments_combined.patch to the forthcoming Sage 4.5.2, which is just 4.5.2.rc1 + #9226, I see

Hunk #1 FAILED at 1036
1 out of 4 hunks FAILED -- saving rejects to file sage/rings/polynomial/infinite_polynomial_ring.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_9443_default_arguments_combined.patch

The reject file's contents:

  • infinite_polynomial_ring.py

     
    10371037        """
    10381038        return False
    10391039
    1040     def is_field(self,**kwds):
     1040    def is_field(self, *args, **kwds):
    10411041        """
    1042         Since Infinite Polynomial Rings must have at least one generator, they
    1043         have infinitely many variables and thus never are fields.
     1042        Return ``False``, since an infinite polynomial ring has at least one
     1043        generator and hence infinitely many variables.
     1044
     1045        EXAMPLES::
     1046
     1047            sage: R.<x, y> = InfinitePolynomialRing(QQ)
     1048            sage: R.is_field()
     1049            False
     1050
    10441051
    10451052        TESTS::
    10461053

Can someone rebase the patch when it's convenient? It might be sufficient to work from #9114.

Changed 11 years ago by niles

rebased against #9114 (for 4.5.2); apply only this patch.

comment:10 in reply to: ↑ 9 Changed 11 years ago by niles

  • Status changed from needs_work to needs_review

should now apply cleanly, although I admit I haven't had time to test it thoroughly.

Changed 11 years ago by mpatel

Restore commit string. Apply only this patch.

comment:11 Changed 11 years ago by mpatel

  • Authors changed from niles to Niles Johnson
  • Status changed from needs_review to positive_review

Thanks! The new patch applies cleanly to 4.5.3.alpha0. I've attached V2, which simply restores the earlier fixed commit message.

comment:12 Changed 11 years ago by mpatel

  • Merged in set to sage-4.6.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.