-
Notifications
You must be signed in to change notification settings - Fork 116
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
Fix method ambiguity error of NaNMath.pow
#690
Conversation
Hmm seems not sufficient... |
src/methods.jl
Outdated
@@ -98,6 +98,9 @@ end | |||
@number_methods(BasicSymbolic{<:Number}, term(f, a), term(f, a, b), skipbasics) | |||
@number_methods(BasicSymbolic{<:LiteralReal}, term(f, a), term(f, a, b), onlybasics) | |||
|
|||
# Fix method ambiguity issue in NaNMath >= 1.0.2 | |||
NaNMath.pow(x::BasicSymbolic{<:Number}, y::Integer) = x^y |
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.
NaNMath.pow(x::BasicSymbolic{<:Number}, y::Integer) = x^y | |
NaNMath.pow(x::BasicSymbolic{<:Number}, y::Integer) = maketerm(typeof(x), NaNMath.pow, [x, y], metadata(x)) |
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.
We want to avoid NaNMath
in that case in NaNMath < 1.0.2 and on NaNMath >= 1.0.2 an Integer
as second argument falls back to ^
anyway. So I think using ^
is the better and safer approach, regardless of the NaNMath version.
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.
Then can we add this to @number_methods
?
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.
I did but I guess incorrectly. The original error is gone but it seems it causes https://github.com/JuliaSymbolics/SymbolicUtils.jl/actions/runs/12708708876/job/35426212172?pr=690#step:6:885.
In fact, this should instead be a change to |
Actually, never mind 😅 Symbolics is the only one with the ambiguity, right? Because |
I think a couple of the tests need to be updated |
NaNMath 1.0.2 introduced a method ambiguity error.