Repository: sympy/sympy
Pull request: Fixed as_poly() by making it raise error with unsupported data types eg: tuples
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #18038 


#### Brief description of what is fixed or changed

The commit fixes Issue #18038 in which as_poly() would work with tuples and give wrong output. This was fixed by making it an attribute of Expr as tuples are not a subclass Expr. Tests for the same were added  in sympy/core/tests/test_expr.py and sympy/integrals/tests/test_integrals.py . This ultimately fixed the issue where integrate((x, x)) would give a wrong result.


#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* core
    * Moved .as_poly() from Basic to Expr.
<!-- END RELEASE NOTES -->


Problem statement:
What is the result of integrate of a tuple (i.e. integrate((x, x)))?
I just accidently discovered unexpected (at least, for me) behaviour of `integrate` if its first argument is a tuple:

```
In [60]: integrate((x, x))
Out[60]:
 3
x     2
── + x
2

In [61]: integrate((x, x, x))
Out[61]:
 4    3
x    x     2
── + ── + x
3    2

In [62]: integrate((x, 1))
Out[62]:
 3
x
── + x
2
```
It seems to be undocumented and I cannot figure out what does it do. Probably, documentation should be updated to address this.

Hints from issue discussion:
Looks like this is entirely accidental. It stems from

```
>>> Tuple(x, x).as_poly(x)
Poly(x*x + x, x, domain='ZZ[x]')
```

which doesn't seem like it should be something that works to me. 
`.as_poly()` should probably reject invalid polynomial representations:
```
--- a/sympy/core/basic.py
+++ b/sympy/core/basic.py
@@ -785,7 +785,7 @@ def as_poly(self, *gens, **args):
         from sympy.polys import Poly, PolynomialError
 
         try:
-            poly = Poly(self, *gens, **args)
+            poly = Poly.new(self, *gens, **args)
 
             if not poly.is_Poly:
                 return None
```
I am working on the issue and can't figure out why `Poly.new()` should be preferred and why 
https://github.com/sympy/sympy/blob/52069d42f4d6df1811269f4bf8c00da93e02fae6/sympy/polys/polytools.py#L114-L118

is not working
I agree with @jksuom. There's also a secondary issue here which is that `Poly((x, x), x)` creates a polynomial where x is both a generator and contained in the coefficients. That should give an error IMO.
But in the suggested solution `new()` cannot perform the functionalities that `__new__()` can, like constructing polynomials from dictionaries, lists etc. Or am I failing to understand something here.
> constructing polynomials from dictionaries, lists etc.

These are not SymPy's expressions. Only those have the attribute `.as_poly()`. `__new__()` accepts also those but that is for internal use in the polys module. `.new()` is designed for those cases where the arguments have to be checked for validity. (In fact, I think that it might have been better to have a private method `_new()` for internal use and `__new__()` could have done what `new()` now does.)
> In fact, I think that it might have been better to have a private method `_new()` for internal use and `__new__()` could have done what `new()` now does.

I can do that but I have a couple of doubts regarding `new()`, it accepts an object of the class `Basic` and compares it with class `DMP`. Shouldn't it always raise a `PolynomialError` when called from 
`as_poly()` ?https://github.com/sympy/sympy/blob/52069d42f4d6df1811269f4bf8c00da93e02fae6/sympy/polys/polytools.py#L130-L132

and it returns an object of class `Basic`
https://github.com/sympy/sympy/blob/52069d42f4d6df1811269f4bf8c00da93e02fae6/sympy/polys/polytools.py#L136-L141
Souldn't the returned object be of class `Poly`?
I'm also unclear why as_poly exists on Basic instead of just Expr. Are there useful non-Expr classes that can be converted to Poly, especially if we disable it working with Tuple?
> Are there useful non-Expr classes that can be converted to Poly,

I cannot think of any.
@jksuom I think that in your solution where `new()` is called instead of `__new__()` the result would be a Basic class object compared with the DMP class. And instead of returning a Poly object, a Basic class object would be returned.

Commit message: Fixed as_poly() by making it raise error with unsupported data types eg:
tuples

The commit fixes Issue #18038 in which as_poly() would work with tuples and give wrong output. This was fixed by making it an attribute of Expr as tuples are not a subclass Expr. Tests for the same were added  in sympy/core/tests/test_expr.py and sympy/integrals/tests/test_integrals.py . This ultimately fixed the issue where integrate((x, x)) would give a wrong result.

Patch to review:
diff --git a/sympy/core/basic.py b/sympy/core/basic.py
index 79deff1978e8..a11249c9399f 100644
--- a/sympy/core/basic.py
+++ b/sympy/core/basic.py
@@ -766,33 +766,7 @@ def _sorted_args(self):
         return self.args
 
 
-    def as_poly(self, *gens, **args):
-        """Converts ``self`` to a polynomial or returns ``None``.
 
-        >>> from sympy import sin
-        >>> from sympy.abc import x, y
-
-        >>> print((x**2 + x*y).as_poly())
-        Poly(x**2 + x*y, x, y, domain='ZZ')
-
-        >>> print((x**2 + x*y).as_poly(x, y))
-        Poly(x**2 + x*y, x, y, domain='ZZ')
-
-        >>> print((x**2 + sin(y)).as_poly(x, y))
-        None
-
-        """
-        from sympy.polys import Poly, PolynomialError
-
-        try:
-            poly = Poly(self, *gens, **args)
-
-            if not poly.is_Poly:
-                return None
-            else:
-                return poly
-        except PolynomialError:
-            return None
 
     def as_content_primitive(self, radical=False, clear=True):
         """A stub to allow Basic args (like Tuple) to be skipped when computing
diff --git a/sympy/core/expr.py b/sympy/core/expr.py
index b1e85ead5f64..edbf84e9c248 100644
--- a/sympy/core/expr.py
+++ b/sympy/core/expr.py
@@ -1076,6 +1076,34 @@ def as_ordered_factors(self, order=None):
         """Return list of ordered factors (if Mul) else [self]."""
         return [self]
 
+    def as_poly(self, *gens, **args):
+        """Converts ``self`` to a polynomial or returns ``None``.
+
+        >>> from sympy import sin
+        >>> from sympy.abc import x, y
+
+        >>> print((x**2 + x*y).as_poly())
+        Poly(x**2 + x*y, x, y, domain='ZZ')
+
+        >>> print((x**2 + x*y).as_poly(x, y))
+        Poly(x**2 + x*y, x, y, domain='ZZ')
+
+        >>> print((x**2 + sin(y)).as_poly(x, y))
+        None
+
+        """
+        from sympy.polys import Poly, PolynomialError
+
+        try:
+            poly = Poly(self, *gens, **args)
+
+            if not poly.is_Poly:
+                return None
+            else:
+                return poly
+        except PolynomialError:
+            return None
+
     def as_ordered_terms(self, order=None, data=False):
         """
         Transform an expression to an ordered list of terms.
diff --git a/sympy/core/tests/test_expr.py b/sympy/core/tests/test_expr.py
index 1ed84afb9af8..b42da78d4b58 100644
--- a/sympy/core/tests/test_expr.py
+++ b/sympy/core/tests/test_expr.py
@@ -934,6 +934,9 @@ def test_as_poly_as_expr():
 
     assert p.as_poly() == p
 
+    raises(AttributeError, lambda: Tuple(x, x).as_poly(x))
+    raises(AttributeError, lambda: Tuple(x ** 2, x, y).as_poly(x))
+
 
 def test_nonzero():
     assert bool(S.Zero) is False
diff --git a/sympy/integrals/tests/test_integrals.py b/sympy/integrals/tests/test_integrals.py
index 8c2fdeaff9df..a9a8bdfae0bc 100644
--- a/sympy/integrals/tests/test_integrals.py
+++ b/sympy/integrals/tests/test_integrals.py
@@ -215,6 +215,10 @@ def test_issue_3560():
     assert integrate(1/sqrt(x)**3, x) == -2/sqrt(x)
 
 
+def test_issue_18038():
+    raises(AttributeError, lambda: integrate((x, x))
+
+
 def test_integrate_poly():
     p = Poly(x + x**2*y + y**3, x, y)
 


Code review comments:
diff_hunk: @@ -215,6 +215,10 @@ def test_issue_3560():
     assert integrate(1/sqrt(x)**3, x) == -2/sqrt(x)
 
 
+def test_issue_18038():
+    raises(AttributeError, lambda: integrate((x, x))
path: sympy/integrals/tests/test_integrals.py
text: @user1:
One more parenthesis should be appended. Now a syntax error results.

@author:
I made the changes but Travis is still showing some error

@user1:
That was an internet connection issue. I have restarted the job.