fix: fix fast_substitute folding array of symbolics
#1398
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 #1398 +/- ##
=========================================
+ Coverage 3.95% 6.44% +2.48%
=========================================
Files 51 51
Lines 4824 4841 +17
=========================================
+ Hits 191 312 +121
+ Misses 4633 4529 -104 ☔ View full report in Codecov by Sentry. |
|
The NaNMath error is trivial. The others I can't seem to replicate outside of running tests. |
|
This will also fix MTK. |
src/num.jl
Outdated
| @@ -25,6 +25,8 @@ SymbolicUtils.@number_methods( | |||
| Num(f(value(a), value(b))), | |||
| [conj, real, transpose] | |||
| ) | |||
|
|
|||
| NaNMath.pow(x::Num, y::Integer) = wrap(NaNMath.pow(unwrap(x), y)) | |||
There was a problem hiding this comment.
We dont't want that. It should just fall back to ^.
There was a problem hiding this comment.
Why not? @number_methods above already defines NaNMath.pow this way
There was a problem hiding this comment.
Because NaNMath.pow returns floats for integer arguments in NaNMath < 1.0.2 which breaks eg Catalyst (see the issue in SymbolicUtils) and NaNMath >= 1.0.2 just falls back to ^ if the second argument is an Integer. So directly using ^ is the only way that works with all NaNMath versions and is what NaNMath does anyway in newer versions.
There was a problem hiding this comment.
Thanks for the clarification, I just saw your comments on the SymbolicUtils PR.
AayushSabharwal
force-pushed
the
as/fast-substitute-array
branch
from
4cdb222 to
1ea3df5
Compare
|
@n0rbed can you look into what's going on with the solver tests? |
ChrisRackauckas
merged commit 3d8ce96
into
JuliaSymbolics:master
|
As far as I have debugged, the failing solver tests are due to hashconsing, but they are not the fault of hashconsing. In some places, the solver creates expressions with |
|
Can you handle that then? |
|
I can try, but it'll take a while 😅 there's a lot of code to track down where the |
Required for SciML/ModelingToolkit.jl#3282