Skip to content

Commit

Permalink
Edit ptiede makiefix (#793)
Browse files Browse the repository at this point in the history 
* Correct the name and change surface2 to use the correct conversion trait for that plot

* Fix test problems

* Tried to be more generic with the conversion but getting stackoverflow

* Hack together something to see if tests pass

* Fix _lookup_to_vector and _lookup_to_interval (#776)

* Fix lookup bug

* Add some tests

* recipes

---------

Co-authored-by: Paul Tiede <ptiede91@gmail.com>
  • Loading branch information
@rafaqz @ptiede
rafaqz and ptiede authored Sep 1, 2024
1 parent 1a63704 commit cee41d1
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 13 deletions.
49 changes: 38 additions & 11 deletions ext/DimensionalDataMakie.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ for (f1, f2) in _paired(:plot => :heatmap, :heatmap, :image, :contour, :contourf
x=nothing, y=nothing, colorbarkw=(;), attributes...
) where T
replacements = _keywords2dimpairs(x, y)
A1, A2, args, merged_attributes = _surface2(A, attributes, replacements)
A1, A2, args, merged_attributes = _surface2(A, $f2, attributes, replacements)
p = if $(f1 == :surface)
# surface is an LScene so we cant pass attributes
p = Makie.$f2(args...; attributes...)
Expand All @@ -161,27 +161,38 @@ for (f1, f2) in _paired(:plot => :heatmap, :heatmap, :image, :contour, :contourf
x=nothing, y=nothing, colorbarkw=(;), attributes...
)
replacements = _keywords2dimpairs(x, y)
_, _, args, _ = _surface2(A, attributes, replacements)
_, _, args, _ = _surface2(A, $f2, attributes, replacements)
# No ColourBar in the ! in-place versions
return Makie.$f2!(axis, args...; attributes...)
end
function Makie.$f1!(axis, A::Observable{<:AbstractDimMatrix};
x=nothing, y=nothing, colorbarkw=(;), attributes...
)
replacements = _keywords2dimpairs(x,y)
args = lift(x->_surface2(x, attributes, replacements)[3], A)
args = lift(x->_surface2(x, $f2, attributes, replacements)[3], A)
p = Makie.$f2!(axis, lift(x->x[1], args),lift(x->x[2], args),lift(x->x[3], args); attributes...)
return p
end
end
end

function _surface2(A, attributes, replacements)
function _surface2(A, plotfunc, attributes, replacements)
# Array/Dimension manipulation
A1 = _prepare_for_makie(A, replacements)
lookup_attributes, newdims = _split_attributes(A1)
A2 = _restore_dim_names(set(A1, map(Pair, newdims, newdims)...), A, replacements)
args = Makie.convert_arguments(Makie.VertexGrid(), A2)
P = Plot{plotfunc}
args = Makie.convert_arguments(P, A2)
# PTrait = Makie.conversion_trait(P, A2)

This comment has been minimized.

Sign in to view

Copy link
@asinghvi17

asinghvi17 Sep 3, 2024

Collaborator

@lazarusA this is why contour/f is failing!

This comment has been minimized.

Sign in to view

Copy link
@rafaqz

rafaqz Sep 3, 2024

Author Owner

Why exactly?

This comment has been minimized.

Sign in to view

Copy link
@asinghvi17

asinghvi17 Sep 3, 2024

Collaborator

It's not converting according to the trait, and convert_arguments by default returns the input. For example,

julia> convert_arguments(Contourf, :asdkjasd)
(:asdkjasd,)

But if we defined a convert_arguments for VertexGrid(), this would not pick it up, and the args passed in to the plot would be wrong.

This comment has been minimized.

Sign in to view

Copy link
@asinghvi17

asinghvi17 Sep 3, 2024 •
edited
Loading

Collaborator

I guess this needs a full refactor of the way DD hijacks $(f)!, to not do that. Only problem is that expand_dimensions can't consume attributes I think. Will have to think of a way around this, maybe going deeper into the Makie API.

This comment has been minimized.

Sign in to view

Copy link
@rafaqz

rafaqz Sep 3, 2024

Author Owner

Ok... I don't really get how these Makie traits work, but it seems kind of inconsistent.

What can we do here to fix?

I'm kind of regretting adding these methods at all now... Makie just needs to allow adding Attributes inside convert_arguments and we could delete most of this junk. Its a maintenance nightmare honestly.

This comment has been minimized.

Sign in to view

Copy link
@rafaqz

rafaqz Sep 3, 2024 •
edited
Loading

Author Owner

Sorry you said that you commented before me - but the entire reason we have these methods at all is that Makie doesn't let us set Attributes any other way. And attributes is most of what DimensionalData adds... e.g. dimension names!
# status = Makie.got_converted(P, PTrait, converted)

# if status === true
# args = converted
# else
# args = Makie.convert_arguments(P, converted...)
# end



# Plot attribute generation
dx, dy = DD.dims(A2)
Expand Down Expand Up @@ -215,7 +226,7 @@ for (f1, f2) in _paired(:plot => :volume, :volume, :volumeslices)
@doc $docstring
function Makie.$f1(A::AbstractDimArray{<:Any,3}; x=nothing, y=nothing, z=nothing, attributes...)
replacements = _keywords2dimpairs(x, y, z)
A1, A2, args, merged_attributes = _volume3(A, attributes, replacements)
A1, A2, args, merged_attributes = _volume3(A, $f2, attributes, replacements)
p = Makie.$f2(args...; merged_attributes...)
if !isnothing(p.axis.scene[OldAxis])
p.axis.scene[OldAxis][:names, :axisnames] = map(DD.label, DD.dims(A2))
Expand All @@ -224,18 +235,18 @@ for (f1, f2) in _paired(:plot => :volume, :volume, :volumeslices)
end
function Makie.$f1!(axis, A::AbstractDimArray{<:Any,3}; x=nothing, y=nothing, z=nothing, attributes...)
replacements = _keywords2dimpairs(x, y, z)
_, _, args, _ = _volume3(A, attributes, replacements)
_, _, args, _ = _volume3(A, $f2, attributes, replacements)
return Makie.$f2!(axis, args...; attributes...)
end
end
end

function _volume3(A, attributes, replacements)
function _volume3(A, plotfunc, attributes, replacements)
# Array/Dimension manipulation
A1 = _prepare_for_makie(A, replacements)
_, newdims = _split_attributes(A1)
A2 = _restore_dim_names(set(A1, map(Pair, newdims, newdims)...), A, replacements)
args = Makie.convert_arguments(Makie.VolumeLike(), A2)
args = Makie.convert_arguments(Plot{plotfunc}, A2)

# Plot attribute generation
user_attributes = Makie.Attributes(; attributes...)
Expand Down Expand Up @@ -355,10 +366,19 @@ Makie.plottype(::AbstractDimVector) = Makie.Scatter
Makie.plottype(::AbstractDimMatrix) = Makie.Heatmap
Makie.plottype(::AbstractDimArray{<:Any,3}) = Makie.Volume

# TODO this needs to be added to Makie
# Makie.to_endpoints(x::Tuple{Makie.Unitful.AbstractQuantity,Makie.Unitful.AbstractQuantity}) = (ustrip(x[1]), ustrip(x[2]))
# Makie.expand_dimensions(::Makie.PointBased, y::IntervalSets.AbstractInterval) = (keys(y), y)

# Conversions
function Makie.convert_arguments(t::Type{<:Makie.AbstractPlot}, A::AbstractDimMatrix)
A1 = _prepare_for_makie(A)
xs, ys = map(_lookup_to_vector, lookup(A1))
tr = Makie.conversion_trait(t, A)
if tr isa ImageLike
xs, ys = map(_lookup_to_interval, lookup(A1))
else
xs, ys = map(_lookup_to_vector, lookup(A1))
end
return xs, ys, last(Makie.convert_arguments(t, parent(A1)))
end
function Makie.convert_arguments(t::Makie.PointBased, A::AbstractDimVector)
Expand Down Expand Up @@ -388,7 +408,14 @@ function Makie.convert_arguments(
xs, ys = map(_lookup_to_vector, lookup(A1))
return xs, ys, last(Makie.convert_arguments(t, parent(A1)))
end
function Makie.convert_arguments(t::Makie.VolumeLike, A::AbstractDimArray{<:Any,3})
function Makie.convert_arguments(t::Makie.VolumeLike, A::AbstractDimArray{<:Any,3})
A1 = _prepare_for_makie(A)
xs, ys, zs = map(_lookup_to_interval, lookup(A1))
# the following will not work for irregular spacings
return xs, ys, zs, last(Makie.convert_arguments(t, parent(A1)))
end

function Makie.convert_arguments(t::Type{Plot{Makie.volumeslices}}, A::AbstractDimArray{<:Any,3})
A1 = _prepare_for_makie(A)
xs, ys, zs = map(_lookup_to_vector, lookup(A1))
# the following will not work for irregular spacings
Expand Down
2 changes: 1 addition & 1 deletion test/plotrecipes.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ end
# Units are broken in Makie ?
# fig, ax, _ = M.volume(A3u)
# M.volume!(ax, A3u)

fig, ax, _ = M.volumeslices(A3)
M.volumeslices!(ax, A3)
# Need to manually specify colorrange
Expand Down
2 changes: 1 addition & 1 deletion test/runtests.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ using DimensionalData, Test, Aqua, SafeTestsets
Aqua.test_project_extras(DimensionalData)
Aqua.test_stale_deps(DimensionalData)
Aqua.test_deps_compat(DimensionalData)

@time @safetestset "interface" begin include("interface.jl") end
@time @safetestset "metadata" begin include("metadata.jl") end
@time @safetestset "name" begin include("name.jl") end
Expand Down

0 comments on commit cee41d1

Please sign in to comment.