Open fatteneder opened 1 year ago
I don't like the range thing at all because I think the code still doesn't check if the number of elements matches the number of pixels, and instead just uses the endpoints like an interval. This invites bugs, maybe we should refactor to interval notation .. ?
... maybe we should refactor to interval notation
You mean instead of image(1:400, 1:200, img)
to write image(1..400, 1..200, image)
? I would love that. But is that used anywhere else already?
Perhaps we should then also add conversions for [1,400], [1,200]
etc.
Both of those work in GLMakie. image
doesn't care about values other than min/max at all it seems. For heatmaps [low, high]
works alongside an array of explicit values
image doesn't care about values other than min/max at all it seems Which is why I think it should be deprecated
maybe rather (1, 400)
than [1,400]
because it's nice that you can dispatch-force two-element tuples but not arrays?
maybe rather (1, 400)
yes.
Perhaps also using more precise variable names in the doc string could already help a lot. Consider
julia> ?image
image(xrange, yrange, img)
image(img)
Plots an image `img` on range `xrange, yrange` (defaults to dimensions of `img`).
Namely,
xrange, yrange
instead of x,y
; at least I always associate x,y
to positions,img
instead of image
for the data, because using image(1:400,1:200,image)
would throw even an error.Just a quick note here, Makie currently uses 0..m, 0..n. (i.e. 0..400, 0..200). This makes each pixel a 1x1 square unit of space in the region up to 400.
The docstring has changed to https://github.com/MakieOrg/Makie.jl/blob/8fc77b261ef66ad526722cc3cbfb3df73b6da693/MakieCore/src/basic_plots.jl#L210 Not sure if that's clear enough to guess input types correctly...
Current doc string:
Just spent 10 min to figure out that
x, y
must be<:AbstractRange
. I think this could be formulated more clearly, maybe just annotate the signature? E.g.image(x::AbstractRange, y::AbstractRange, image)
. Same would apply toheatmap
. Although, other plot functions aren't that specific either ...Alternatively we could just add another MWE to the docs page.