Resolve isequal(Num, ForwardDiff.Dual) ambiguity #1247
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1247 +/- ##
===========================================
+ Coverage 8.30% 30.39% +22.08%
===========================================
Files 46 46
Lines 4549 4570 +21
===========================================
+ Hits 378 1389 +1011
+ Misses 4171 3181 -990 ☔ View full report in Codecov by Sentry. |
|
The test failures seem unrelated to this. |
| # https://github.com/JuliaSymbolics/Symbolics.jl/issues/1246 | ||
| @testset "isequal type ambiguity" begin | ||
| @variables x | ||
| xfunc(xval) = isequal(x, xval) ? xval : xval |
There was a problem hiding this comment.
| xfunc(xval) = isequal(x, xval) ? xval : xval | |
| xfunc(xval) = isequal(x, xval) ? x : xval |
I don't quite understand this test? In both cases the derivative is respect to the value, so even if isequal is wrong this would pass. It needs to have a "bad" branch that would be wrong/error
There was a problem hiding this comment.
Thanks, I agree. I was very focused on simply ensuring it does not crash. I will improve it a bit and add a bad branch.
|
@ChrisRackauckas Do you think this one is a little better? |
|
Will check it out tomorrow morning |
|
@ChrisRackauckas This test isnt written well in master. its now fixed in the latest PR im working on. If you can dont merge it yet though |
This is another attempt at fixing SciML/ModelingToolkit.jl#2997 by addressing the actual root cause of the issue, which is described with a minimal example in #1246. Following #1036, I have also copied ForwardDiff's
isequal()definition to resolve the type ambiguity.