Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fixes, integration of solve_interms_ofvar and some changes #1357

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

n0rbed
Copy link
Member

@n0rbed n0rbed commented Nov 11, 2024

New stuff:

julia> symbolic_solve(1/x, x)
1-element Vector{Float64}:
 Inf
julia> symbolic_solve(x^1.5, x)
1-element Vector{BigFloat}:
 0.0

"Strict" kwarg for solve_univar @AayushSabharwal :

julia> Symbolics.solve_univar(x^1.5 + x, x)
ERROR: AssertionError: Not a polynomial

julia> Symbolics.solve_univar(x^1.5 + x, x, strict=false)
1-element Vector{SymbolicUtils.BasicSymbolic{Number}}:
roots_of(x + x^1.5, x)

Documentation and integration of solve_interms_ofvar was also added. ia_rules.jl was moved to special_cases.jl because thats more representative i think, since the solve_interms_ofvar case isnt "isolation and attraction" - its just some algebraic manipulation method we use in a special case @ChrisRackauckas (let me know what you think).

@n0rbed
Copy link
Member Author

n0rbed commented Nov 11, 2024

A next todo would be using ia_solve recursively instead of symbolic_solve in solve_interms_ofvar to make it more flexible

@n0rbed
Copy link
Member Author

n0rbed commented Nov 11, 2024

If im not mistaken, this fixes #1356

@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 91.89189% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.07%. Comparing base (5af597a) to head (0368c3e).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
src/solver/main.jl 77.77% 2 Missing ⚠️
src/solver/postprocess.jl 87.50% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1357       +/-   ##
===========================================
+ Coverage    3.98%   79.07%   +75.09%     
===========================================
  Files          50       50               
  Lines        4771     4880      +109     
===========================================
+ Hits          190     3859     +3669     
+ Misses       4581     1021     -3560     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sumiya11
Copy link
Contributor

One data point:

In Maple 2024 solve(1/x = 0); returns nothing and solve(x^1.5 = 0); returns 0.0.

@n0rbed
Copy link
Member Author

n0rbed commented Nov 11, 2024

In Maple 2024 solve(1/x = 0); returns nothing

@sumiya11 Do you think this is preferable? i have a feeling that "nothing" could be confused with "we cant solve this".

@AayushSabharwal
Copy link
Contributor

In julia 1/0 is Inf and 1/Inf is 0. I feel like Inf is the better return value here.

Comment on lines +265 to +270
- strict (optional): Bool that enables/disables strict assert if input expression is a univariate polynomial or not. If strict=true and expression is not a polynomial, `solve_univar` throws an assertion error.

# Examples

"""
function solve_univar(expression, x; dropmultiplicity=true)
function solve_univar(expression, x; dropmultiplicity=true, strict=true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should solve_univar be renamed to solve_univar_poly ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solve_multivar should be renamed to solve_multivar_poly aswell then - i'll do it when i get the time

@sumiya11
Copy link
Contributor

In Maple 2024 solve(1/x = 0); returns nothing

@sumiya11 Do you think this is preferable? i have a feeling that "nothing" could be confused with "we cant solve this".

I guess Inf is fine. My thoughts are:

Inf for 1/x is fine since substituting Inf back gives zero. In that vein, solve(x / x^2) should return nothing. So it is not clear to me how useful Inf could be in general.

On the other hand, returning nothing for 1/x is easy and correct (in a sense that the roots of P(x) / Q(x) = 0 is a subset of the roots of P(x) = 0).

@sumiya11
Copy link
Contributor

sumiya11 commented Nov 11, 2024

Another data point:

>>> sympy.solve(1/x)
[]

>>> sympy.solve(x**(1.5))
[0.0]

UPD: Reduce

julia> "solve(1/x, x)" |> rcall
"{}"

julia> "solve(x^(1.5), x)" |> rcall
"{x=0,x=0}"

@n0rbed
Copy link
Member Author

n0rbed commented Nov 12, 2024

@sumiya11 Looks like its universal
Mathematica 14.0

In[1] := Solve[1/x == 0]
Out[1]= {}

Matlab

>> syms x
>> solve(1/x == 0, x)
 
ans =
 
Empty sym: 0-by-1

@AayushSabharwal
Copy link
Contributor

AayushSabharwal commented Nov 12, 2024

If this renaming is happening and may happen in the future, it would be good to either document the API as experimental (so users know it can undergo breaking changes) and/or make the old names fall back to the new ones.

EDIT: Meant to reply to #1357 (comment)

@n0rbed
Copy link
Member Author

n0rbed commented Nov 12, 2024

Yes i'll deprecate them and make them call the new renamed versions

@n0rbed
Copy link
Member Author

n0rbed commented Nov 12, 2024

Does the output of 1/x matter in MTK btw? which would u prefer @AayushSabharwal

@AayushSabharwal
Copy link
Contributor

It doesn't particularly matter for MTK

@ChrisRackauckas ChrisRackauckas merged commit c31c3fd into JuliaSymbolics:master Nov 18, 2024
9 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants