Solving trignometric equations which previously gave error #13941
Conversation
…g in term of tan()
sympy/solvers/solveset.py
Outdated
| denominators.append(Rational(c).q) | ||
|
|
||
| x = Dummy('x') | ||
| f = f.subs(symbol, 2*lcm(denominators)*x) |
There was a problem hiding this comment.
precompute and store as LCM_den = ilcm(*denominators) (or similar) and use when needed below.
sympy/solvers/solveset.py
Outdated
|
|
||
| else : | ||
| raise NotImplementedError(filldedent(''' | ||
| Solution to this kind of triginometric equations |
sympy/solvers/solveset.py
Outdated
| except ValueError: | ||
| raise ValueError("give up, we can't solve if this is not a polynomial in x") | ||
| if poly_ar.degree() > 1: # degree >1 still bad | ||
| raise ValueError("degree of variable inside polynomail should not exceed one") |
jashan498
changed the title
sympy/solvers/solveset.py
Outdated
| if poly_ar.degree() == 0: # degree 0, no x, don't care | ||
| continue | ||
| c = poly_ar.all_coeffs(x)[0] # got the coefficient of x | ||
| denominators.append(Rational(c).q) |
There was a problem hiding this comment.
Does it make sense to remove the gcd of the numerators, too?
There was a problem hiding this comment.
I don't think numerators would matter. We just want to change the fraction coefficient of x into an integer. If any of the coefficients of x remains fractional, then expand_trig() cant convert it into tan(x) , for doing so there is no need to do anything with numerator.
eg: m = sin(S(3)*x/2) + sin(5*x)
replacing x with 2*2*s will make the coefficients as 6*s and 20*s respectively which can be easily converted to respective tan with an integer coefficient by expand_trig(). 3 and 5 being there don't gonna affect it because they have nothing to do with coefficient being fractional.
So, I don't think there would be any role of numerator in here : )
There was a problem hiding this comment.
If you rewrite the expression you suggested in terms of tan and replace the x with 4s or 8s (either of which clears the numerator) you end up with a very different set of equations to solve. By not being economical with the replacement you would be forced to solve a larger set of equations -- of higher order:
>>> [degree(i.subs(tan(s),y),y) for i in (factor(expand_trig(eq.rewrite(tan).subs(x,8*s)))).as_numer_denom()[0].args]
[0,1,1,6,6,12,12,1]
>>> [degree(i.subs(tan(s),y),y) for i in (factor(expand_trig(eq.rewrite(tan).subs(x,4*s)))).as_numer_denom()[0].args]
[0,6,12,1]
Granted, in this case, there would be no reason to use 8 instead of 4 but if the original expression were sin(S(3)*x/2) + sin(6*x) it would be better to rewrite replacing x with 4s/3 instead of 4s, wouldn't it?
There was a problem hiding this comment.
Yeah, you are right. It's indeed more economical. Added gcd for numerators too.
sympy/solvers/solveset.py
Outdated
| denominators.append(Rational(c).q) | ||
|
|
||
| x = Dummy('x') | ||
| d_lcm = lcm(denominators) |
There was a problem hiding this comment.
I think you will want to use the integer-optimized ilcm and igcd; then compute const = Rational(2)*d_lcm/n_gcd and use that when doing the subs at lines 474 and 489 and 491 (at least I think it has to be used in all 3 places -- if no tests fail then a test that affirms this need should be added).
|
I killed the test run -- let's not waste a test for whitespace alone. You didn't added a test for an expression that fails in master but not here that needs the gcd of the numerators extracted: does |
|
No, it returns a |
If you make no mistake in the code, no...but you did and no test caught it because the gcd was 1 so it won't show up until someone tries an equation that doesn't have a gcd of 1. ;-) So if the equation I gave you returns a solution in this PR you can confirm (with a print statement in the code) that the gcd is not 1, remove the print statement, add the test, check locally that the test passes, and then commit it. WITHOUT CLOSING this PR, locally just |
|
@smichr, I used |
|
+1 Looking good! |
Fixes #13899
Previously all trigonometric equations were solved by changing all trigino functions in their respective Euler form. This many times leads to the introduction of two variables leading to formation of multivariate polynomials leading to a
PolyError.So, i added
_solve_trig2()helper function (as explained by @normalhuman in the issue) which gets called everytime the primary helper function (_solve_trig1()) cant solve the equation.It first coverts coefficients of all
angleinto integer and then write all trigonometric functions in form oftanusing half-angle substitution. After thattanis substituted with a 'Dummy' variable, thus transforming it into a polynomial which is at the end solved to get the solution.Some examples which give an error in current master :
>>> x=Symbol('x')>>> solveset(2*tan(x)*sin(x) + 1, x)>>> x=Symbol('x')>>> solveset(2*cos(x)*cos(2*x) - 1, x)