solvers: Fixed NoneType Error #18324

sachin-4099 · 2020-01-13T19:15:32Z

Brief description of what is fixed or changed:

Fixes an error in solvers.solvers.unrad() where it would return a TypeError: cannot unpack non-iterable NoneType object

Unrad() now returns the simplified numerator even when there are radicals left in the denominator, but since the numerator is free of radicals it seems logical to return the simplified numerator.

Example:

1) eq = -8*x**2/(9*(x**2 - 1)**(S(4)/3)) + 4/(3*(x**2 - 1)**(S(1)/3))
   a, b = unrad(eq)
OR
2) solveset(-8*x**2/(9*(x**2 - 1)**(S(4)/3)) + 4/(3*(x**2 - 1)**(S(1)/3)), x, S.Complexes)

Used to return: TypeError: cannot unpack non-iterable NoneType object

Now returns: 1) (4*x**2 - 12, []) and 2) FiniteSet(sqrt(3), -sqrt(3))

Other comments:
Added tests

Release Notes:

solvers
- Fixed NoneType Error Bug in solvers.solvers.unrad()

sympy-bot · 2020-01-13T19:15:34Z

✅

Hi, I am the SymPy bot (v149). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

solvers
- Fixed NoneType Error Bug in solvers.solvers.unrad() (#18324 by @sachin-4099)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

Fixes #17882
Closes #18026

**Brief description of what is fixed or changed:**

Fixes an error in solvers.solvers.unrad() where it would return a `TypeError: cannot unpack non-iterable NoneType object`

Unrad() now returns the simplified numerator even when there are radicals left in the denominator, but since the numerator is free of radicals it seems logical to return the simplified numerator.

**Example:**
```
1) eq = -8*x**2/(9*(x**2 - 1)**(S(4)/3)) + 4/(3*(x**2 - 1)**(S(1)/3))
   a, b = unrad(eq)
OR
2) solveset(-8*x**2/(9*(x**2 - 1)**(S(4)/3)) + 4/(3*(x**2 - 1)**(S(1)/3)), x, S.Complexes)
```
**Used to return:** `TypeError: cannot unpack non-iterable NoneType object`

**Now returns:** `1) (4*x**2 - 12, []) and 2) FiniteSet(sqrt(3), -sqrt(3))`

**Other comments:**
**Added tests**

**Release Notes:**
<!-- BEGIN RELEASE NOTES -->
* solvers
   *  Fixed NoneType Error Bug in solvers.solvers.unrad()

Update

The release notes on the wiki have been updated.

sachin-4099 · 2020-01-13T19:17:53Z

Please review the PR @jksuom @oscarbenjamin .
If any changes are required please suggest
Otherwise the PR is ready to be merged.

codecov · 2020-01-13T20:59:14Z

Codecov Report

Merging #18324 into master will increase coverage by 0.534%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##            master    #18324       +/-   ##
=============================================
+ Coverage   75.002%   75.536%   +0.534%     
=============================================
  Files          649       643        -6     
  Lines       167347    167325       -22     
  Branches     39418     39440       +22     
=============================================
+ Hits        125514    126392      +878     
+ Misses       36299     35403      -896     
+ Partials      5534      5530        -4

czgdp1807 · 2020-01-14T08:15:04Z

sympy/solvers/tests/test_solveset.py

@@ -178,6 +178,14 @@ def test_issue_11536():
    assert solveset(0**x - 1, x, S.Reals) == FiniteSet(0)


+def test_issue_17882():
+    equation = solveset(-8*x**2/(9*(x**2 - 1)**(4/3)) + 4/(3*(x**2 - 1)**(1/3)), x, S.Complexes)
+    equation = str(equation)


We should avoid using str for testing.

Should I implement an alternative approach??

Use rational numbers for the coefficients, like S(4)/3. You should be able to compare the solution exactly.

Can you rewrite the equation in the required way, so that I get a better idea?

Specifically, you should write the equation like
solveset(-8*x**2/(9*(x**2 - 1)**(S(4)/3)) + 4/(3*(x**2 - 1)**(S(1)/3)), x, S.Complexes)

It looks like the issue is not resolved 'well'.
If you just want to stop investigating further and get over with the unevaluated result like
ConditionSet(x, Eq(-8x**2(x2 - 1)(-1.33333333333333)/9 + 4*(x2 - 1)(-0.333333333333333)/3, 0), S.Complexes), I would merge this soon.

But I would rather update the original post and keep the thread open, and make sure that it should resolve

Some logic that leads to infinite recursions

It should actually 'solve' the equation rather than returning conditionset.

Actually, it looks like this is not resolving anything because your original test case
solveset(-8*x**2/(9*(x**2 - 1)**(4/3)) + 4/(3*(x**2 - 1)**(1/3)), x, S.Complexes)
works as same in the master.

It looks like you should actually get
solveset(-8*x**2/(9*(x**2 - 1)**(S(4)/3)) + 4/(3*(x**2 - 1)**(S(1)/3)), x, S.Complexes)
to work

Ok, no issue.
But in my opinion, we can merge this and keep the thread open for further improvements.

No, unfortunately for the reason in #18324 (comment)

ok, i didn't observe this, i will start working on this immediately.

sachin-4099 · 2020-01-15T09:21:07Z

@jksuom in the original issue: #17882, was regarding a Nonetype error, but since the issue is already resolved, should we go ahead with this PR, and try to get:
solveset(-8*x**2/(9*(x**2 - 1)**(S(4)/3)) + 4/(3*(x**2 - 1)**(S(1)/3)), x, S.Complexes)
to work or the result shown is fine.
Shall we open a new issue regarding the maximum recursion depth exceeded error we encountered?

jksuom · 2020-01-15T10:23:41Z

It seems that unrad should not have returned None as there are radicals in the expression. However, after some simplifications they are only in the denominator d:

sympy/sympy/solvers/solvers.py

Line 3448 in f3c949f

eq, d = eq.as_numer_denom()

I think that unrad should return the simplified numerator eq even there are no radicals left. Maybe something like this:

--- a/sympy/solvers/solvers.py
+++ b/sympy/solvers/solvers.py
@@ -3460,7 +3460,7 @@ def _take(d, take_int_pow):
     # check for trivial case
     # - already a polynomial in integer powers
     if all(_Q(g) == 1 for g in gens):
-        return
+        return eq, []
     # - an exponent has a symbol of interest (don't handle)
     if any(g.as_base_exp()[1].has(*syms) for g in gens):
         return

sachin-4099 · 2020-01-15T10:58:50Z

a, b = unrad(eq)
was giving: TypeError: cannot unpack non-iterable NoneType object

whre eq = -8*x**2/(9*(x**2 - 1)**(4/3)) + 4/(3*(x**2 - 1)**(1/3))

Now after the above mentioned change by @jksuom, the issue is resolved.

sachin-4099 · 2020-01-15T10:59:27Z

@jksuom shall i go ahead with this or anything else needs to be changed?

sachin-4099 · 2020-01-15T11:19:24Z

It seems that unrad should not have returned None as there are radicals in the expression. However, after some simplifications they are only in the denominator d:

sympy/sympy/solvers/solvers.py

Line 3448 in f3c949f

eq, d = eq.as_numer_denom()

I think that unrad should return the simplified numerator eq even there are no radicals left. Maybe something like this:
--- a/sympy/solvers/solvers.py
+++ b/sympy/solvers/solvers.py
@@ -3460,7 +3460,7 @@ def _take(d, take_int_pow):
     # check for trivial case
     # - already a polynomial in integer powers
     if all(_Q(g) == 1 for g in gens):
-        return
+        return eq, []
     # - an exponent has a symbol of interest (don't handle)
     if any(g.as_base_exp()[1].has(*syms) for g in gens):
         return

This is causing testcases to fail in solvers\ode.py

sachin-4099 · 2020-01-15T11:21:06Z

This is caused due to change in return type of unrad()

jksuom · 2020-01-15T12:23:18Z

The equations of ode.py involve derivatives. Those are removed from poly.gens by _take to form gens. It seems that the algorithm expect None to be returned if there are such generators (i.e.,not all poly.gens are in gens). This can be handled by, for example,

--- a/sympy/solvers/solvers.py
+++ b/sympy/solvers/solvers.py
@@ -3460,7 +3460,10 @@ def _take(d, take_int_pow):
     # check for trivial case
     # - already a polynomial in integer powers
     if all(_Q(g) == 1 for g in gens):
-        return
+        if len(gens) == len(poly.gens):
+            return eq, []
+        else:
+            return
     # - an exponent has a symbol of interest (don't handle)
     if any(g.as_base_exp()[1].has(*syms) for g in gens):
         return

sachin-4099 · 2020-01-15T12:46:33Z

Still one testcase is failing:
sympy\solvers\solveset.py", line 1744, in sympy.solvers.solveset._transolve
Failed example: pprint(tsolve(f, x), use_unicode=False)

jksuom · 2020-01-15T13:56:41Z

It looks like more conditions have to be added. That tsolve example created generators g with non-integer exponents. I had expected that the condition _Q(g) == 1 would suffice to reject those but apparently that is not true. The following seems to work in this case:

--- a/sympy/solvers/solvers.py
+++ b/sympy/solvers/solvers.py
@@ -3460,7 +3460,11 @@ def _take(d, take_int_pow):
     # check for trivial case
     # - already a polynomial in integer powers
     if all(_Q(g) == 1 for g in gens):
-        return
+        if (len(gens) == len(poly.gens) and
+            all(g.is_rational_function() for g in gens)):
+            return eq, []
+        else:
+            return
     # - an exponent has a symbol of interest (don't handle)
     if any(g.as_base_exp()[1].has(*syms) for g in gens):
         return

It might be possible (or necessary) to use an even more restrictive condition like g.is_polynomial().

sachin-4099 · 2020-01-15T14:28:18Z

Now the tests are passing, but the same error is occuring:
TypeError: cannot unpack non-iterable NoneType object

sachin-4099 · 2020-02-05T17:38:25Z

@jksuom please review the PR, if any changes are required please suggest otherwise the PR is ready to be merged.

jksuom · 2020-02-05T18:09:27Z

Thanks, I think it is ready.

solvers.solveset: Fixed NoneType Error of solvers.solveset()

0662a69

czgdp1807 added solvers solvers.solveset labels Jan 14, 2020

czgdp1807 reviewed Jan 14, 2020

View reviewed changes

sachin-4099 requested a review from jksuom January 15, 2020 09:16

sachin-4099 requested a review from sylee957 January 15, 2020 10:59

sachin-4099 changed the title ~~solvers: Fixed NoneType Error of solvers.solveset()~~ solvers: Fixed NoneType Error Feb 4, 2020

Fixed: NoneType error of solvers.unrad()

d237dce

sachin-4099 removed the request for review from sylee957 February 4, 2020 19:40

Additional Testcase added

c56e359

jksuom merged commit 530df75 into sympy:master Feb 5, 2020

smichr mentioned this pull request Mar 23, 2021

Modified how unrad handled poly expressions #21002

Closed

smichr mentioned this pull request Apr 9, 2021

updates to unrad, continuous_domain, roots_(cubic, quartic, quintic) #21276

Merged

oscarbenjamin mentioned this pull request Aug 21, 2021

Solveset throws TypeError with sqrt in denominator #15350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solvers: Fixed NoneType Error #18324

solvers: Fixed NoneType Error #18324

sachin-4099 commented Jan 13, 2020 •

edited

sympy-bot commented Jan 13, 2020 •

edited

sachin-4099 commented Jan 13, 2020 •

edited

codecov bot commented Jan 13, 2020 •

edited

czgdp1807 Jan 14, 2020

sachin-4099 Jan 14, 2020

asmeurer Jan 14, 2020

sachin-4099 Jan 14, 2020

sylee957 Jan 15, 2020

sylee957 Jan 15, 2020 •

edited

sylee957 Jan 15, 2020 •

edited

sachin-4099 Jan 15, 2020

sylee957 Jan 15, 2020

sachin-4099 Jan 15, 2020

sachin-4099 commented Jan 15, 2020

jksuom commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

jksuom commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

jksuom commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

sachin-4099 commented Feb 5, 2020

jksuom commented Feb 5, 2020

solvers: Fixed NoneType Error #18324

solvers: Fixed NoneType Error #18324

Conversation

sachin-4099 commented Jan 13, 2020 • edited

sympy-bot commented Jan 13, 2020 • edited

sachin-4099 commented Jan 13, 2020 • edited

codecov bot commented Jan 13, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylee957 Jan 15, 2020 • edited

Choose a reason for hiding this comment

sylee957 Jan 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachin-4099 commented Jan 15, 2020

jksuom commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

jksuom commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

jksuom commented Jan 15, 2020

sachin-4099 commented Jan 15, 2020

sachin-4099 commented Feb 5, 2020

jksuom commented Feb 5, 2020

sachin-4099 commented Jan 13, 2020 •

edited

sympy-bot commented Jan 13, 2020 •

edited

sachin-4099 commented Jan 13, 2020 •

edited

codecov bot commented Jan 13, 2020 •

edited

sylee957 Jan 15, 2020 •

edited

sylee957 Jan 15, 2020 •

edited