Meshes.jl icon indicating copy to clipboard operation
Meshes.jl copied to clipboard

Use promotion in constructors for `Ball`, `Sphere`, and `Cylinder`

Open CameronBieganek opened this issue 3 years ago • 4 comments

This ought to work:

julia> c = Ball(Point(0, 0), 1)
ERROR: MethodError: no method matching Ball(::Point2, ::Int64)
Closest candidates are:
  Ball(::Tuple, ::Any) at ~/.julia/packages/Meshes/6EgDO/src/primitives/ball.jl:17
  Ball(::Point{Dim, T}, ::T) where {Dim, T} at ~/.julia/packages/Meshes/6EgDO/src/primitives/ball.jl:13

You could either use promotion in the constructor, or even just wrap radius with float() in the constructor.

Similarly for Sphere, and also for Cylinder:

julia> Cylinder(3)
ERROR: MethodError: no method matching Cylinder(::Int64, ::Segment{3, Float64, Vector{Point3}})
Closest candidates are:
  Cylinder(::T) where T at ~/.julia/packages/Meshes/6EgDO/src/primitives/cylinder.jl:36
  Cylinder(::T, ::Segment{3, T, V} where V<:AbstractArray{Point{3, T}, 1}) where T at ~/.julia/packages/Meshes/6EgDO/src/primitives/cylinder.jl:28
  Cylinder(::T, ::Plane{T}, ::Plane{T}) where T at ~/.julia/packages/Meshes/6EgDO/src/primitives/cylinder.jl:23
Stacktrace:
 [1] Cylinder(radius::Int64)
   @ Meshes ~/.julia/packages/Meshes/6EgDO/src/primitives/cylinder.jl:40
 [2] top-level scope
   @ REPL[129]:1

I've used function signatures like the following before (quoted from your code here).

function Cylinder(radius::T, segment::Segment{3,T}) where {T}

But the problem with signatures like that is that you're missing out on reasonable promotions between number types.

CameronBieganek avatar Jan 21 '22 20:01 CameronBieganek

Definitely! The promotion rule seems appropriate. We want to convert all Integer to Float64 to avoid errors downstream. Exact representaions willl demand a different abstract type that is not Integer.

juliohm avatar Jan 21 '22 20:01 juliohm

@fujiwara-tofu-tm would you like to work on this issue?

juliohm avatar Jun 07 '22 21:06 juliohm

@juliohm sounds fun, I'll give it a shot this weekend

nick-adamowicz avatar Jun 08 '22 00:06 nick-adamowicz

@nick-adamowicz did you have a chance to look into that?

juliohm avatar Jun 23 '22 17:06 juliohm