Change ImageLike and VolumeLike deprecation warnings to errors (#4685)

* hard-deprecate Vector-like Image and Volume conversion * fix dispatch, allow direct conversion of AbstractRange for Volume * fix tests * update Makie tests * update changelog --------- Co-authored-by: Simon <[email protected]>
MakieOrg · Jan 13, 2025 · d3d6eec · d3d6eec
1 parent 22dd3b1
commit d3d6eec
Show file tree

Hide file tree

Showing 10 changed files with 119 additions and 84 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,11 +27,12 @@
 - Added `transform_marker` attribute to meshscatter and changed the default behavior to not transform marker/mesh vertices [#4606](https://github.com/MakieOrg/Makie.jl/pull/4606)
 - Fixed some issues with meshscatter not correctly transforming with transform functions and float32 rescaling [#4606](https://github.com/MakieOrg/Makie.jl/pull/4606)
 - Fixed `poly` pipeline for 3D and/or Float64 polygons that begin from an empty vector [#4615](https://github.com/MakieOrg/Makie.jl/pull/4615).
-- `empty!` GLMakie screen instead of closing, fixing issue with resetted window position [#3881](https://github.com/MakieOrg/Makie.jl/pull/3881)
+- `empty!` GLMakie screen instead of closing, fixing issue with reset window position [#3881](https://github.com/MakieOrg/Makie.jl/pull/3881)
 - Added option to display the front spines in Axis3 to close the outline box [#2349](https://github.com/MakieOrg/Makie.jl/pull/4305)
 - Fixed gaps in corners of `poly(Rect2(...))` stroke [#4664](https://github.com/MakieOrg/Makie.jl/pull/4664)
 - Fixed an issue where `reinterpret`ed arrays of line points were not handled correctly in CairoMakie [#4668](https://github.com/MakieOrg/Makie.jl/pull/4668).
 - Fixed various issues with `markerspace = :data`, `transform_marker = true` and `rotation` for scatter in CairoMakie (incorrect marker transformations, ignored transformations, Cairo state corruption) [#4663](https://github.com/MakieOrg/Makie.jl/pull/4663)
+- Changed deprecation warnings for Vector and Range inputs in `image`, `volume`, `voxels` and `spy` into **errors** [#4685](https://github.com/MakieOrg/Makie.jl/pull/4685)
 - Refactored OpenGL cleanup to run immediately rather than on GC [#4699](https://github.com/MakieOrg/Makie.jl/pull/4699)
 - It is now possible to change the title of a `GLFW.Window` with `GLMakie.set_title!(screen::Screen, title::String)` [#4677](https://github.com/MakieOrg/Makie.jl/pull/4677).
 

diff --git a/ReferenceTests/src/tests/generic_components.jl b/ReferenceTests/src/tests/generic_components.jl
@@ -3,7 +3,7 @@
 @reference_test "picking" begin
     scene = Scene(size = (230, 370))
     campixel!(scene)
-    
+
     sc1 = scatter!(scene, [20, NaN, 20], [20, NaN, 50], marker = Rect, markersize = 20)
     sc2 = scatter!(scene, [50, 50, 20, 50], [20, 50, 80, 80], marker = Circle, markersize = 20, color = [:red, :red, :transparent, :red])
     ms = meshscatter!(scene, [20, NaN, 50], [110, NaN, 110], markersize = 10)
@@ -18,16 +18,16 @@
     hm = heatmap!(scene, [80, 110, 140], [140, 170], [1 4; 2 5; 3 6])
     # mesh coloring should match triangle placements
     m = mesh!(scene, Point2f.([80, 80, 110, 110], [200, 230, 200, 230]), [1 2 3; 2 3 4], color = [1,1,1,2])
-    vx = voxels!(scene, [65, 155], [245, 305], [-1, 1], reshape([1,2,3,4,5,6], (3,2,1)), shading = NoShading)
+    vx = voxels!(scene, (65, 155), (245, 305), (-1, 1), reshape([1,2,3,4,5,6], (3,2,1)), shading = NoShading)
     vol = volume!(scene, 80..110, 320..350, -1..1, rand(2,2,2))
-    
+
     # reversed axis
     i2 = image!(scene, 210..180, 20..50, rand(RGBf, 2, 2))
     s2 = surface!(scene, 210..180, 80..110, [1 2; 3 4], interpolate = false)
     hm2 = heatmap!(scene, [210, 180], [140, 170], [1 2; 3 4])
 
     # for ranged picks
-    m2 = mesh!(scene, 
+    m2 = mesh!(scene,
         Point2f[(190, 330), (200, 330), (190, 340), (200, 340)],
         [1 2 4; 1 4 3]
     )
@@ -52,7 +52,7 @@
     else
         error("picking tests are only meant to run on GLMakie & WGLMakie")
     end
-    
+
     # raw picking tests
     @testset "pick(scene, point)" begin
         @testset "scatter" begin
@@ -108,7 +108,7 @@
             @test pick(scene, 61, 290) == (nothing, 0)
         end
 
-        @testset "text" begin        
+        @testset "text" begin
             @test pick(scene, 15, 320) == (t, 1)
             @test pick(scene, 13, 320) == (nothing, 0)
             # edge checks, further outside due to AA
@@ -131,7 +131,7 @@
                 )
                 @test pick(scene, p) == (nothing, 0)
             end
-            
+
             # cell centered checks
             @test pick(scene,  90, 30) == (i, 1)
             @test pick(scene, 110, 30) == (i, 2)
@@ -145,7 +145,7 @@
             @test pick(scene, 100+1, 35-1) == (i, 2)
             @test pick(scene, 100-1, 35+1) == (i, 4)
             @test pick(scene, 100+1, 35+1) == (i, 5)
-            
+
             @test pick(scene, 120-1, 35-1) == (i, 2)
             @test pick(scene, 120+1, 35-1) == (i, 3)
             @test pick(scene, 120-1, 35+1) == (i, 5)
@@ -166,7 +166,7 @@
                 )
                 @test pick(scene, p) == (nothing, 0)
             end
-            
+
             # cell centered checks
             @test pick(scene,  90,  90) == (s, 1)
             @test pick(scene, 110,  90) == (s, 2)
@@ -180,7 +180,7 @@
             @test pick(scene,  95+1, 95-1) == (s, 2)
             @test pick(scene,  95-1, 95+1) == (s, 4)
             @test pick(scene,  95+1, 95+1) == (s, 5)
-            
+
             @test pick(scene, 125-1, 95-1) == (s, 2)
             @test pick(scene, 125+1, 95-1) == (s, 3)
             @test pick(scene, 125-1, 95+1) == (s, 5)
@@ -201,7 +201,7 @@
                 )
                 @test pick(scene, p) == (nothing, 0)
             end
-            
+
             # cell centered checks
             @test pick(scene,  80, 140) == (hm, 1)
             @test pick(scene, 110, 140) == (hm, 2)
@@ -215,7 +215,7 @@
             @test pick(scene, 96, 154) == (hm, 2)
             @test pick(scene, 94, 156) == (hm, 4)
             @test pick(scene, 96, 156) == (hm, 5)
-            
+
             @test pick(scene, 124, 154) == (hm, 2)
             @test pick(scene, 126, 154) == (hm, 3)
             @test pick(scene, 124, 156) == (hm, 5)
@@ -251,7 +251,7 @@
                 )
                 @test pick(scene, p) == (nothing, 0)
             end
-            
+
             # cell centered checks
             @test pick(scene,  80, 260) == (vx, 1)
             @test pick(scene, 110, 260) == (vx, 2)
@@ -265,15 +265,15 @@
             @test pick(scene,  96, 274) == (vx, 2)
             @test pick(scene,  94, 276) == (vx, 4)
             @test pick(scene,  96, 276) == (vx, 5)
-            
+
             @test pick(scene, 124, 274) == (vx, 2)
             @test pick(scene, 126, 274) == (vx, 3)
             @test pick(scene, 124, 276) == (vx, 5)
             @test pick(scene, 126, 276) == (vx, 6)
         end
-        
+
         @testset "volume" begin
-            # volume doesn't produce indices because we can't resolve the depth of 
+            # volume doesn't produce indices because we can't resolve the depth of
             # the pick
             @test pick(scene,  80, 320)[1] == vol
             @test pick(scene,  79, 320) == (nothing, 0)
@@ -303,7 +303,7 @@
         @testset "linesegments" begin
             @test pick(scene,  5, 280, 10) == (ls, 6)
         end
-        @testset "text" begin        
+        @testset "text" begin
             @test pick(scene, 32, 320, 10) == (t, 1)
             @test pick(scene, 35, 320, 10) == (t, 3)
         end
@@ -393,7 +393,7 @@
     end
 
     #=
-    For Verfication 
+    For Verfication
     Note that the text only marks the index in the picking list. The position
     that is closest (that pick_sorted used) is somewhere else in the marked
     element. Check scene2 to see the pickable regions if unsure
@@ -410,15 +410,15 @@
     end
     scatter!(scene, Vec2f(100, 100), color = :white, strokecolor = :black, strokewidth = 2, overdraw = true)
     text!(
-        scene, ps, text = ["$i" for i in 1:14], 
-        strokecolor = :white, strokewidth = 2, 
+        scene, ps, text = ["$i" for i in 1:14],
+        strokecolor = :white, strokewidth = 2,
         align = (:center, :center), overdraw = true)
     =#
 
     # pick(scene, Rect)
     # grab all indices and generate a plot for them (w/ fixed px_per_unit)
     full_screen = last.(pick(scene, scene.viewport[]))
-    
+
     scene2 = Scene(size = 2.0 .* widths(scene.viewport[]))
     campixel!(scene2)
     image!(scene2, full_screen, colormap = :viridis)

diff --git a/ReferenceTests/src/tests/primitives.jl b/ReferenceTests/src/tests/primitives.jl
@@ -861,21 +861,25 @@ end
     img = [2 0 0 3; 0 0 0 0; 1 1 0 0; 1 1 0 4]
 
     f = Figure(size = (600, 400))
+    to_tuple(x) = (first(x), last(x))
+    to_tuple(x::Makie.Interval) = (Makie.leftendpoint(x), Makie.rightendpoint(x))
 
     for (i, interp) in enumerate((true, false))
         for (j, plot_func) in enumerate((
-            (fp, x, y, cs, interp) -> image(fp, x, y, cs, colormap = :viridis, interpolate = interp),
+            (fp, x, y, cs, interp) -> image(fp, to_tuple(x), to_tuple(y), cs, colormap = :viridis, interpolate = interp),
             (fp, x, y, cs, interp) -> heatmap(fp, x, y, cs, colormap = :viridis, interpolate = interp),
             (fp, x, y, cs, interp) -> surface(fp, x, y, zeros(size(cs)), color = cs, colormap = :viridis, interpolate = interp, shading = NoShading)
         ))
 
             gl = GridLayout(f[i, j])
 
-            a, p = plot_func(gl[1, 1], (1, 4), (1, 4), img, interp)
+            # Test forwards + backwards for each: Tuple, Interval, Range, Vector
+
+            a, p = plot_func(gl[1, 1], 1:4, (1, 4), img, interp)
             hidedecorations!(a)
-            a, p = plot_func(gl[2, 1], (1, 4), 4..1, img, interp)
+            a, p = plot_func(gl[2, 1], [1, 2, 3, 4], 4..1, img, interp)
             hidedecorations!(a)
-            a, p = plot_func(gl[1, 2], (4, 1), (1, 4), img, interp)
+            a, p = plot_func(gl[1, 2], 4:-1:1, 1..4, img, interp)
             hidedecorations!(a)
             a, p = plot_func(gl[2, 2], (4, 1), [4, 3, 2, 1], img, interp)
             hidedecorations!(a)

diff --git a/src/basic_recipes/spy.jl b/src/basic_recipes/spy.jl
@@ -74,8 +74,8 @@ function convert_arguments(::Type{<:Spy}, matrix::AbstractMatrix{T}) where T
 end
 
 function convert_arguments(::Type{<:Spy}, xs, ys, matrix::AbstractMatrix)
-    x = to_endpoints(xs, "x")
-    y = to_endpoints(ys, "y")
+    x = to_endpoints(xs, "x", "Spy")
+    y = to_endpoints(ys, "y", "Spy")
     return (x, y, convert(SparseArrays.SparseMatrixCSC, matrix))
 end
 

diff --git a/src/basic_recipes/voxels.jl b/src/basic_recipes/voxels.jl
@@ -4,9 +4,9 @@ function Makie.convert_arguments(T::Type{<:Voxels}, chunk::Array{<: Real, 3})
 end
 
 function convert_arguments(T::Type{<:Voxels}, xs, ys, zs, chunk::Array{<: Real, 3})
-    xi = Float32.(to_endpoints(xs))
-    yi = Float32.(to_endpoints(ys))
-    zi = Float32.(to_endpoints(zs))
+    xi = Float32.(to_endpoints(xs, "x", Voxels))
+    yi = Float32.(to_endpoints(ys, "y", Voxels))
+    zi = Float32.(to_endpoints(zs, "z", Voxels))
     return convert_arguments(T, xi, yi, zi, chunk)
 end
 

diff --git a/src/conversions.jl b/src/conversions.jl
@@ -371,10 +371,17 @@ function to_endpoints(x::Tuple{<:Real,<:Real})
 end
 to_endpoints(x::Interval) = to_endpoints(endpoints(x))
 to_endpoints(x::EndPoints) = x
-to_endpoints(x::AbstractVector) = to_endpoints((first(x), last(x)))
-function to_endpoints(x, dim)
-    x isa AbstractVector && !(x isa EndPoints) && print_range_warning(dim, x)
-    return to_endpoints(x)
+
+to_endpoints(x::AbstractVector, side, trait) = throw_range_error(x, side, trait)
+to_endpoints(x::Union{Tuple, EndPoints, Interval}, side, trait) = to_endpoints(x)
+
+function throw_range_error(value::T, side, trait) where {T}
+    error(
+        "Encountered `$T` with value $value on side $side in `convert_arguments` for the `$trait`
+        conversion. Using `$T` to specify one dimension of `$trait` is deprecated because `$trait`
+        sides always need exactly two values, start and stop. Use interval notation `start .. stop`,
+        a two-element tuple `(start, stop)` or `Makie.EndPoints(start, stop)` instead."
+    )
 end
 
 function convert_arguments(::GridBased, x::EndPointsLike, y::EndPointsLike,
@@ -411,17 +418,10 @@ function convert_arguments(::CellGrid, x::EndPointsLike, y::EndPointsLike,
     return (EndPoints{Tx}(xe[1] - xstep, xe[2] + xstep), EndPoints{Ty}(ye[1] - ystep, ye[2] + ystep), el32convert(z))
 end
 
-function print_range_warning(side::String, value)
-    @warn "Encountered an `AbstractVector` with value $value on side $side in `convert_arguments` for the `ImageLike` trait.
-        Using an `AbstractVector` to specify one dimension of an `ImageLike` is deprecated because `ImageLike` sides always need exactly two values, start and stop.
-        Use interval notation `start .. stop` or a two-element tuple `(start, stop)` instead."
-end
-
-
 function convert_arguments(::ImageLike, xs::RangeLike, ys::RangeLike,
                            data::AbstractMatrix{<:Union{Real,Colorant}})
-    x = to_endpoints(xs, "x")
-    y = to_endpoints(ys, "y")
+    x = to_endpoints(xs, "x", ImageLike)
+    y = to_endpoints(ys, "y", ImageLike)
     return (x, y, el32convert(data))
 end
 
@@ -470,7 +470,8 @@ end
 
 function convert_arguments(::VolumeLike, x::RangeLike, y::RangeLike, z::RangeLike,
                            data::RealArray{3})
-    return (to_endpoints(x, "x"), to_endpoints(y, "y"), to_endpoints(z, "z"), el32convert(data))
+    return (to_endpoints(x, "x", VolumeLike), to_endpoints(y, "y", VolumeLike),
+            to_endpoints(z, "z", VolumeLike), el32convert(data))
 end
 
 """
@@ -481,7 +482,8 @@ Takes 3 `AbstractVector` `x`, `y`, and `z` and the `AbstractMatrix` `i`, and put
 `P` is the plot Type (it is optional).
 """
 function convert_arguments(::VolumeLike, x::RealVector, y::RealVector, z::RealVector, i::RealArray{3})
-    (to_endpoints(x, "x"), to_endpoints(y, "y"), to_endpoints(z, "z"), el32convert(i))
+    return (to_endpoints(x, "x", VolumeLike), to_endpoints(y, "y", VolumeLike),
+            to_endpoints(z, "z", VolumeLike), el32convert(i))
 end
 
 ################################################################################
@@ -580,7 +582,7 @@ function convert_arguments(::Type{<:Mesh}, geom::GeometryPrimitive{N, T}) where
     # we convert to UV mesh as default, because otherwise the uv informations get lost
     # - we can still drop them, but we can't add them later on
     m = GeometryBasics.expand_faceviews(GeometryBasics.uv_normal_mesh(
-        geom; pointtype = Point{N, float_type(T)}, 
+        geom; pointtype = Point{N, float_type(T)},
         uvtype = Vec2f, normaltype = Vec3f, facetype = GLTriangleFace
     ))
     return (m,)
@@ -646,6 +648,13 @@ function convert_arguments(::Type{<:Arrows}, x::RealVector, y::RealVector, z::Re
     return (vec(points), vec(f_out))
 end
 
+# Note: AbstractRange must be linear
+is_regularly_spaced(x::AbstractRange) = true
+function is_regularly_spaced(x::AbstractVector)
+    delta = x[2] - x[1]
+    return all(i -> x[i] - x[i-1] ≈ delta, 3:length(x))
+end
+
 """
     convert_arguments(P, x, y, z, f)::(Vector, Vector, Vector, Matrix)
 
@@ -654,16 +663,21 @@ spanned by `x`, `y` and `z`, and puts `x`, `y`, `z` and `f(x,y,z)` in a Tuple.
 
 `P` is the plot Type (it is optional).
 """
-function convert_arguments(VL::VolumeLike, x::RealVector, y::RealVector, z::RealVector, f::Function)
+function convert_arguments(::VolumeLike, x::RealVector, y::RealVector, z::RealVector, f::Function)
     if !applicable(f, x[1], y[1], z[1])
         error("You need to pass a function with signature f(x, y, z). Found: $f")
     end
+    # Verify grid regularity
+    is_regularly_spaced(x) || throw_range_error(x, "x", VolumeLike)
+    is_regularly_spaced(y) || throw_range_error(y, "y", VolumeLike)
+    is_regularly_spaced(z) || throw_range_error(z, "z", VolumeLike)
+
     _x, _y, _z = ntuple(Val(3)) do i
         A = (x, y, z)[i]
         return reshape(A, ntuple(j -> j != i ? 1 : length(A), Val(3)))
     end
-    # TODO only allow  unitranges to map over since we dont support irregular x/y/z values
-    return (map(to_endpoints, (x, y, z))..., el32convert.(f.(_x, _y, _z)))
+
+    return (map(v -> to_endpoints((first(v), last(v))), (x, y, z))..., el32convert.(f.(_x, _y, _z)))
 end
 
 function convert_arguments(P::Type{<:AbstractPlot}, r::RealVector, f::Function)

diff --git a/test/boundingboxes.jl b/test/boundingboxes.jl
@@ -144,7 +144,7 @@ end
     @test bb.origin ≈ Point3f(0)
     @test bb.widths ≈ Vec3f(10.0, 10.0, 0)
 
-    fig, ax, p = image(1..0, 1:10, rand(10, 10))
+    fig, ax, p = image(1..0, 1..10, rand(10, 10))
     bb = boundingbox(p)
     @test bb.origin ≈ Point3f(0, 1, 0)
     @test bb.widths ≈ Vec3f(1.0, 9.0, 0)

diff --git a/test/conversions.jl b/test/conversions.jl
@@ -351,16 +351,24 @@ end
 
     o3 = Float32.(m3)
 
+    t1 = (1, 10)
+    t2 = (1, 6)
+
     xx = convert_arguments(Image, m3)
     xx == ((0.0f0, 10.0f0), (0.0f0, 6.0f0), o3)
     @testset "ImageLike conversion" begin
         @test convert_arguments(Image, m3) == ((0.0f0, 10.0f0), (0.0f0, 6.0f0), o3)
-        @test convert_arguments(Image, v1, r2, m3) == ((1.0f0, 10.0f0), (1.0f0, 6.0f0), o3)
-        @test convert_arguments(Image, i1, v2, m3) == ((1.0f0, 10.0f0), (1.0f0, 6.0f0), o3)
-        @test convert_arguments(Image, v3, i1, m3) == ((10, 1), (1, 10), o3)
-        @test convert_arguments(Image, v1, i3, m3) == ((1, 10), (10, 1), o3)
+        @test convert_arguments(Image, i1, i2, m3) == ((1.0, 10.0), (1.0, 6.0), o3)
+        @test convert_arguments(Image, i1, t2, m3) == ((1.0, 10.0), (1.0, 6.0), o3)
+        @test convert_arguments(Image, t1, t2, m3) == ((1.0, 10.0), (1.0, 6.0), o3)
+
+        @test_throws ErrorException convert_arguments(Image, v1, r2, m3)
+        @test_throws ErrorException convert_arguments(Image, i1, v2, m3)
+        @test_throws ErrorException convert_arguments(Image, v3, i1, m3)
+        @test_throws ErrorException convert_arguments(Image, v1, i3, m3)
+
+        # TODO: Should probably fail because it's not accepted by backends?
         @test convert_arguments(Image, m1, m2, m3) === (m1, m2, m3)
-        @test convert_arguments(Heatmap, m1, m2) === (m1, m2)
     end
 
     @testset "VertexGrid conversion" begin
@@ -376,6 +384,7 @@ end
     end
 
     @testset "CellGrid conversion" begin
+        @test convert_arguments(Heatmap, m1, m2) === (m1, m2)
         o1 = (0.5, 10.5)
         o2 = (0.5, 6.5)
         or1 = (0.5:1:10.5)