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

Towards a nicer Makie recipe #720

Closed
asinghvi17 opened this issue Jun 5, 2024 · 10 comments · Fixed by #747 or #769
Closed

Towards a nicer Makie recipe #720

asinghvi17 opened this issue Jun 5, 2024 · 10 comments · Fixed by #747 or #769
Labels

Comments

@asinghvi17
Copy link
Collaborator

It occurs to me that dimensional axes are dimensional, and we can use Makie's new unit integration to deal with them.

For example, instead of defining conversions in convert_arguments, we might also define expand_dimensions for dimensional arrays, so that Makie can pick up on the dimension of the unit. Defining a custom dim conversion might also allow axis modifications like default labels, without the need to override the basic plot! function.

This isn't particularly urgent, but would be (a) nice to have and (b) pave the way for more such integration in the Makie ecosystem.

@asinghvi17
Copy link
Collaborator Author

I just ran into an issue trying to plot a Raster using GeoMakie :D

Consider the following:

using RasterDataSources, Rasters, ArchGDAL, CairoMakie, GeoMakie
ras = Raster(EarthEnv{HabitatHeterogeneity}, :homogeneity) # habitat homogeneity to neighbouring pixel
surface(ras[:homogeneity]; axis = (; type = GeoAxis))

This causes an error:

ERROR: MethodError: no method matching setindex!(::Nothing, ::Tuple{String, String}, ::Symbol, ::Symbol)
Stacktrace:
 [1] surface(A::Raster{…}; x::Nothing, y::Nothing, colorbarkw::@NamedTuple{}, attributes::@Kwargs{})
   @ DimensionalDataMakie ~/.julia/dev/DimensionalData/ext/DimensionalDataMakie.jl:143
 [2] top-level scope
   @ ~/.julia/dev/GeoMakie/examples/rasters.jl:13
Some type information was truncated. Use `show(err)` to see complete types.

because the plotting code assumes that surface is always plotted in a LScene, and more importantly it doesn't query what kind of axis has been created. This can be solved independently, but it's something which I feel could be solved more cleanly with this approach.

@rafaqz
Copy link
Owner

rafaqz commented Jun 10, 2024

This all sounds good to me. But there are potential problems like: we add a Colorbar to some plots, set axis names manually, and add our own attributes to Attributes.

I think we will need an example PR to show if this is viable or not, I suspect currently its not.

Edit: but we should do it anyway, and maybe just keep our own methods until the other things can be moved somewhere else too.

@asinghvi17
Copy link
Collaborator Author

asinghvi17 commented Jun 23, 2024

FYI,

function Makie.expand_dimensions(::Makie.ImageLike, raster::AbstractRaster)
    return Makie.convert_arguments(Makie.ImageLike(), raster)
end

just works. I ran into this issue with GeoMakie's meshimage recently while trying to plot a Raster through it.

We can also avoid evaling things and go through create_plot once MakieOrg/Makie.jl#3974 lands. That gets past the axis issue, but not the colorbar.

@rafaqz
Copy link
Owner

rafaqz commented Jun 24, 2024

Are you saying we just need to add that method?

I don't totally follow like what does "just works" mean exactly here

@asinghvi17
Copy link
Collaborator Author

Yeah we just need to add expand dimensions for each trait to make DimArray handling generic across traits!

@asinghvi17
Copy link
Collaborator Author

function Makie.convert_arguments(t::Type{<:Makie.AbstractPlot}, A::AbstractMatrix)
A1 = _prepare_for_makie(A)
xs, ys = map(parent, lookup(A1))
return xs, ys, last(Makie.convert_arguments(t, parent(A1)))
end
seems like piracy if I understand correctly?

@rafaqz
Copy link
Owner

rafaqz commented Jun 24, 2024

Ah should probably be AbstractDimMatrix?

Maybe a typo

@asinghvi17
Copy link
Collaborator Author

I'll fix in the PR and see if anything breaks :D

@rafaqz
Copy link
Owner

rafaqz commented Jun 24, 2024

Ok beat me to it

@asinghvi17 asinghvi17 reopened this Jun 24, 2024
@asinghvi17
Copy link
Collaborator Author

Unfortunately #747 doesn't seem to solve the issue of dimensional axes. I can't actually see where the stack overflow is happening though.

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