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

[nilesjohnson]

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'

Component: algebra

Keywords: infinite polynomial ring

Author: Niles Johnson

Reviewer: Simon King

Merged: sage-4.6.alpha1

Issue created by migration from https://trac.sagemath.org/ticket/9443

[nilesjohnson]

Attachment: trac_9943_default_arguments.patch.gz

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

[nilesjohnson]

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

[simon-king-jena]

Work Issues: Add ticket number to commit message

[simon-king-jena]

comment:4

Attachment: trac_9943_default_arguments_doctests.patch.gz

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

[simon-king-jena]

comment:5

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

[simon-king-jena]

Reviewer: Simon King

[simon-king-jena]

comment:6

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.

[nilesjohnson]

Attachment: trac_9443_default_arguments_combined.patch.gz

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

[nilesjohnson]

comment:7

Thanks! The combined patch should now be complete.

[nilesjohnson]

Changed work issues from Add ticket number to commit message to none

[qed777]

comment:9

Applying attachment: 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
+++ infinite_polynomial_ring.py
@@ -1037,10 +1037,17 @@
         """
         return False
 
-    def is_field(self,**kwds):
+    def is_field(self, *args, **kwds):
         """
-        Since Infinite Polynomial Rings must have at least one generator, they
-        have infinitely many variables and thus never are fields.
+        Return ``False``, since an infinite polynomial ring has at least one
+        generator and hence infinitely many variables.
+
+        EXAMPLES::
+
+            sage: R.<x, y> = InfinitePolynomialRing(QQ)
+            sage: R.is_field()
+            False
+
 
         TESTS::
 

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

[nilesjohnson]

Attachment: trac_9443_default_arguments_combined_rebased.patch.gz

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

[nilesjohnson]

comment:10

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

[qed777]

Attachment: trac_9443_default_arguments_combined_rebased.2.patch.gz

Restore commit string. Apply only this patch.

[qed777]

comment:11

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

[qed777]

Changed author from niles to Niles Johnson

[qed777]

Merged: sage-4.6.alpha1