Skip to content
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

promote_type is not supposed to be overloaded #3

Closed
nsajko opened this issue Apr 18, 2024 · 3 comments · Fixed by #4
Closed

promote_type is not supposed to be overloaded #3

nsajko opened this issue Apr 18, 2024 · 3 comments · Fixed by #4

Comments

@nsajko
Copy link

nsajko commented Apr 18, 2024

Several methods are added for promote_type in src/TinyHugeNumbers.jl. Xref JuliaLang/julia#54138

@bvdmitri
Copy link
Member

@nsajko so I changed the definition as you suggested, but I actually got obscure ambiguity errors, which weren't present before.

For example, can you suggest a workaround for this?

Error: MethodError: promote_rule(::Type{Union{TinyHugeNumbers.HugeNumber, T
│ inyHugeNumbers.TinyNumber}}, ::Type{ForwardDiff.Dual{ForwardDiff.Tag{typeof
│ (Main.var"##WeaveSandBox#796".f), Float64}, Float64, 12}}) is ambiguous.

https://github.com/ReactiveBayes/RxInfer.jl/actions/runs/8787364895/job/24112422726#step:7:1793

If I change from

Base.promote_rule(::Type{Union{TinyNumber, HugeNumber}}, ::Type{T}) where {T} = T

to

Base.promote_rule(::Type{T}, ::Type{Union{TinyNumber, HugeNumber}}) where {T} = T

I got another ambiguity error, but in tests

MethodError: promote_rule(::Type{BigFloat}, ::Type{Union{HugeNumber, TinyNumber}}) is ambiguous.
  
  Candidates:
    promote_rule(::Type{BigFloat}, ::Type{<:Real})
      @ Base.MPFR mpfr.jl:412
    promote_rule(::Type{T}, ::Type{Union{HugeNumber, TinyNumber}}) where T
      @ TinyHugeNumbers ~/.julia/dev/TinyHugeNumbers.jl/src/TinyHugeNumbers.jl:123
  
  Possible fix, define
    promote_rule(::Type{BigFloat}, ::Type{Union{HugeNumber, TinyNumber}})
  
  Stacktrace:
   [1] promote_type(::Type{BigFloat}, ::Type{Union{HugeNumber, TinyNumber}})

So as far as I understand overloading promote_type is not advised due to possible ambiguity errors? Why do we have more ambiguity errors with overloading promote_rule?

@nsajko
Copy link
Author

nsajko commented Apr 22, 2024

Base.promote_rule(::Type{Union{TinyNumber, HugeNumber}}, ::Type{T}) where {T} = T

Don't use Union, define two methods instead. Union{A,B} will always be less specific than A. Does that fix the issues?

So as far as I understand overloading promote_type is not advised due to possible ambiguity errors? Why do we have more ambiguity errors with overloading promote_rule?

Overloading promote_type causes ambiguity that you can't reliably fix. Overloading promote_rule is like overloading any other function, it may cause ambiguity if you're not being careful.

@bvdmitri
Copy link
Member

Don't use Union, define two methods instead. Union{A,B} will always be less specific than A. Does that fix the issues?

No, that doesn't fully resolve the problem because separate methods were already defined earlier before you opened the issue.

The requirement for additional methods for promote_type arose due to a specific use case:

clamp(0.0, tiny, huge)

Ideally, this should return a value of Float64. However, without the extra 3-argument promote_type method, it returns tiny (which isn't Float64). The issue here lies in clamp attempting to promote tiny and huge first, which lack a meaningful type to promote to, except for their Union. Previously, the 3-argument promote_type method addressed this.

I also tried this implementation and it seems to work just fine, but this fix broke [tiny, huge] use case (it couldn't properly promote the array eltype). Nevertheless, what are your thoughts on this solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants