Simplifying acos(0/1), asin(0,1) and anything + 0 #1264
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 #1264 +/- ##
===========================================
+ Coverage 8.30% 80.46% +72.15%
===========================================
Files 46 47 +1
Lines 4549 4663 +114
===========================================
+ Hits 378 3752 +3374
+ Misses 4171 911 -3260 ☔ View full report in Codecov by Sentry. |
src/solver/postprocess.jl
Outdated
| if any(isequal(oper, o) for o in opers) && isempty(Symbolics.get_variables(x)) | ||
| val = eval(Symbolics.toexpr(x)) | ||
| for i in eachindex(exacts) | ||
| exact_val = eval(Symbolics.toexpr(exacts[i])) | ||
| if isapprox(exact_val, val, atol=1e-6) | ||
| return exacts[i] | ||
| elseif isapprox(-exact_val, val, atol=1e-6) | ||
| return -exacts[i] | ||
| end |
There was a problem hiding this comment.
this is easier to extend id think, just add more exacts into exacts and the loop will check if the asin, acos, or atan evaluate into it
There was a problem hiding this comment.
done, and changed eval -> symbolic_to_float
src/solver/postprocess.jl
Outdated
| @@ -122,3 +135,33 @@ function postprocess_root(x) | |||
| end | |||
| x # unreachable | |||
| end | |||
|
|
|||
| function check_trig_consts(x) | |||
There was a problem hiding this comment.
Missing a docstring, and should add to the docs
Also, is the name correct? For example, why not add e to this?
There was a problem hiding this comment.
Also adding this to the docs is a bit weird. Used only once and the user wouldnt really care. Some other functions like filter_poly are much more interesting. What do you think?
There was a problem hiding this comment.
A general function for taking floats to common symbolic constants has lots of other uses?
There was a problem hiding this comment.
its the reverse but yes ok makes sense
There was a problem hiding this comment.
This conversation was not resolved. Why was it merged?
There was a problem hiding this comment.
check the latest couple of commits, added a small docstring and made the array a const
as discussed here, acos(0) should be simplified.