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:  sage4.6 
Component:  algebra  Keywords:  infinite polynomial ring 
Cc:  Merged in:  sage4.6.alpha1  
Authors:  Niles Johnson  Reviewers:  Simon King 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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)
Change History (17)
Changed 11 years ago by
comment:1 Changed 11 years ago by
 Description modified (diff)
comment:2 Changed 11 years ago by
 Description modified (diff)
comment:3 Changed 11 years ago by
 Status changed from new to needs_review
Changed 11 years ago by
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 followup: ↓ 6 Changed 11 years ago by
 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
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 ; followup: ↓ 7 Changed 11 years ago by
 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
compined patch replacing the previous two; patch name and commit message fixed
comment:7 in reply to: ↑ 6 Changed 11 years ago by
Thanks! The combined patch should now be complete.
comment:8 Changed 11 years ago by
 Work issues Add ticket number to commit message deleted
comment:9 followup: ↓ 10 Changed 11 years ago by
 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
1037 1037 """ 1038 1038 return False 1039 1039 1040 def is_field(self, **kwds):1040 def is_field(self, *args, **kwds): 1041 1041 """ 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 1044 1051 1045 1052 TESTS:: 1046 1053
Can someone rebase the patch when it's convenient? It might be sufficient to work from #9114.
comment:10 in reply to: ↑ 9 Changed 11 years ago by
 Status changed from needs_work to needs_review
should now apply cleanly, although I admit I haven't had time to test it thoroughly.
comment:11 Changed 11 years ago by
 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
 Merged in set to sage4.6.alpha1
 Resolution set to fixed
 Status changed from positive_review to closed
add argument 'proof' with default value False to is_field and is_integral_domain