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

weird construction of rationals #49757

Closed
nsajko opened this issue May 11, 2023 · 8 comments
Closed

weird construction of rationals #49757

nsajko opened this issue May 11, 2023 · 8 comments

Comments

@nsajko
Copy link
Contributor

nsajko commented May 11, 2023

Should a non-concrete type parameter be kept even though the numerator and denominator get promoted?

For example, it seems like in this case typeof(q) should return Int8, because there's a promotion of the members anyway.

julia> q = Rational{Union{Bool,Int8,Int16}}(true, Int8(3))
1//3

julia> typeof(q)
Rational{Union{Bool, Int16, Int8}}

julia> typeof(numerator(q))
Int8

julia> typeof(denominator(q))
Int8

julia> versioninfo()
Julia Version 1.10.0-DEV.1221
Commit 9201414b3d8 (2023-05-05 17:36 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × AMD Ryzen 3 5300U with Radeon Graphics
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, znver2)
  Threads: 1 on 8 virtual cores

I think this could be fixed by merely changing this line:

return checked_den(T, T(num), T(den))

to return checked_den(T(num), T(den))

@Seelengrab
Copy link
Contributor

While I personally agree, apparently this is a big no-no. @timholy

@nsajko
Copy link
Contributor Author

nsajko commented May 11, 2023

The difference in this case is that the promotion happens anyway. That is, in this case the type parameter is simply unnecessarily imprecise.

@nsajko
Copy link
Contributor Author

nsajko commented May 11, 2023

So by @timholy logic we should throw an error in an example like Rational{Union{Bool,Int8,Int16}}(true, Int8(3))? Although I'm not sure whether that would be backwards-compatible.

@timholy
Copy link
Member

timholy commented May 11, 2023

Huge no-no. MyType{T}(args...) is constructor syntax, and you have to have a way of constructing the specific object requested. It's not just a function call. If you want a function call, this should be either MyType(args...) (OK if MyType is parametric) or mytype(args...) (when it's not parametric).

It's important to be able to construct precise types. Imagine I'm passing that object into a type-unstable function and I've provided a typeassert that insists on that precise type? That's perfectly legal code, might be out there in the wild, but suddenly we'd have no way of constructing it.

Moreover, in lots of places we use MyType{T}(args...) to force conversions: you don't have to get picky about the types of the args. A nice example is from ColorTypes:

julia> using FixedPointNumbers, ColorTypes

julia> col = RGB{N0f8}(1, 0.2, 0.4)
RGB{N0f8}(1.0,0.2,0.4)

julia> dump(col)
RGB{N0f8}
  r: N0f8
    i: UInt8 0xff
  g: N0f8
    i: UInt8 0x33
  b: N0f8
    i: UInt8 0x66

I can use the easy-to-type floating-point literals or mixed types for the three arguments (as illustrated here) and still easily get the precise 24-bit color I want. The type of the requested output takes precedence over the type of the inputs, because we're using constructor-syntax.

From what I can tell, you're thinking of MyType{Union{R,S}} as "either MyType{R} or MyType{S}", but that's not true: the latter is Union{MyType{R}, MyType{S}}. You can write a constructor for that type too if you want:

julia> struct MyType{T}
           x::T
       end

julia> Union{MyType{Int},MyType{Float64}}(x::Real) = MyType(int_or_f64(x))

julia> int_or_f64(x::Integer) = Int(x)
int_or_f64 (generic function with 1 method)

julia> int_or_f64(x::Real) = Float64(x)
int_or_f64 (generic function with 2 methods)

julia> Union{MyType{Int},MyType{Float64}}(0xff)
MyType{Int64}(255)

Of course you could also call this mytype(x) if you prefer that syntax.

@timholy
Copy link
Member

timholy commented May 11, 2023

So by @timholy logic we should throw an error in an example like Rational{Union{Bool,Int8,Int16}}(true, Int8(3))?

Definitely not. It should return a Rational{Union{Bool,Int8,Int16}} if it's possible to construct that type from the inputs via convert (including calls to convert via promote).

@nsajko
Copy link
Contributor Author

nsajko commented May 11, 2023

What about the fact that Rational{Union{Bool,Int8,Int16}}(true, Int8(3)) promotes true to Int8? Should we try to prevent that?

@Seelengrab
Copy link
Contributor

That's ok - the constructor only guarantees that the numerator/denominator are <: Union{Bool, Int8, UInt8} after all. The promotion only happens due to gcd (and the subsequent division) promoting. I guess Bool as either the first or second argument could be specialized to save some operations, but in the Union case, the resulting type instability is likely to be worse for downstream code. The field access will never be inferred as Bool, only as the Union.

@nsajko
Copy link
Contributor Author

nsajko commented May 11, 2023

OK, closing then.

@nsajko nsajko closed this as completed May 11, 2023
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

No branches or pull requests

3 participants