-
Notifications
You must be signed in to change notification settings - Fork 156
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
feat: make ia_solve
modular
#1348
feat: make ia_solve
modular
#1348
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1348 +/- ##
===========================================
+ Coverage 6.69% 78.93% +72.23%
===========================================
Files 47 50 +3
Lines 4618 4860 +242
===========================================
+ Hits 309 3836 +3527
+ Misses 4309 1024 -3285 ☔ View full report in Codecov by Sentry. |
@eval fundamental_period(::typeof($fn)) = 360.0 | ||
end | ||
|
||
fundamental_period(::typeof(cospi)) = 2.0 | ||
|
||
for fn in [tand, cotd] | ||
@eval fundamental_period(::typeof($fn)) = 180.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason that these are floats? any downsides for making them ints? curious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning was that due to how BasicSymbolic
works, calling fundamental_period
will always be dynamic dispatch but if all the return types are Float64
, Julia should be able to infer that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks
test/solver.jl
Outdated
expr = sec(x ^ 2 + 4x + 4) ^ 3 - 3 | ||
roots = ia_solve(expr, x) | ||
@test length(roots) == 6 # 2 quadratic roots * 3 roots from cbrt(3) | ||
roots = ia_solve(expr, x; complex_roots = false) | ||
@test length(roots) == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you have the time, maybe also check that these roots' numerical values also holds by evaluating them and comparing them to the known answer from mathematica/matlab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tests under this too
This is really good! i like it; if you get bored of your work in julia> Symbolics.ia_solve(-2a + c + 1 / c, c)
┌ Warning: This expression cannot be solved with the methods available to ia_solve. Try a numerical method instead.
└ @ Symbolics ~/code/julia/Symbolics.jl/src/solver/ia_main.jl:27
ERROR: MethodError: no method matching iterate(::Nothing) |
@ChrisRackauckas this is good to merge |
This attempts to:
ia_solve
to handle any function that implements the inverse interface instead of hardcoding a list.ia_solve
to handle periodic functions that are notsin
,cos
ortan
.ia_solve
(specifically, the way it handles periodic functions and complex roots)@n0rbed I would really appreciate your review on this