support images with reversed axes

[aplavin]

Description

The whole pipeline almost-supported them already, here I just make a few minor fixes where the assumption of increasing values was baked in.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• Added unit tests for new algorithms, conversion methods, etc.

[aplavin]

should it go to the changelog? not a new feature, just fixing existing behavior, making it more uniform and consistent

[SimonDanisch]

Yeah definitely changelog worthy I'd say

[aplavin]

added!
anything else remains to do?

[aplavin]

bump...

[ffreyer]

Added this refimg:

This works for me across GLMakie, CairoMakie and WGLMakie for heatmap with ranges. It doesn't with image in general, Intervals (e.g. 4..1 errors) and Vectors (e.g. [4,3,2,1] doesn't do anything). I think those would all be TODOs before merging, and the refimg test should then also include them. (And we should have one for image too)

[aplavin]

It doesn't with image in general, Intervals (e.g. 4..1 errors) and Vectors (e.g. [4,3,2,1] doesn't do anything)

What exactly doesn't work?

julia> using CairoMakie
julia> fig = Figure()
julia> X = rand(10, 10)
julia> image(fig[1,1], 0..1, 1:10, X)
julia> image(fig[1,2], 1..0, 1:10, X)
julia> fig

gives (with this PR):

L/R images are clearly flipped, as expected.

[ffreyer]

Oh I missed your response somehow. I got things a bit mixed up in my comment. Testing again I get:

Heatmap:

• reversed Intervals error
• reverse ranges work
• reversed vector works

Image:

• reverse Interval works
• reverse range doesn't work
• reversed vector doesn't work

Thinking about it again it's probably not necessary to get ranges and vectors to work for Image since they are deprecated. Interval in heatmap should be fixed though. And the refimg should test everything that works, i.e. reversed Interval, Range and Vector for heatmap and reversed Interval for image.

Edit: Next two commits resolve this

[ffreyer]

@SimonDanisch @jkrumbiegel Is it fine to switch away from minimum and maximum to enable reverse image/heatmap axes? Should we consider this breaking?

(This is fixing the points I made in the last comment)

[ffreyer]

If this change is fine we can merge this

[SimonDanisch]

Is that in the code path, that already warns for vectors?

[SimonDanisch]

Yeah I guess it is... So we don't allow vectors for images anymore anyways 🤷
It's not yet erroring, but should already warn to not use it.
So shouldn't be breaking

[ffreyer]

We have two functions with that change here:

• to_interval affecting Image, Volume (errors in data_limits with reversed axis though)
• to_linspace affecting PointBased w/ (Interval, Interval, Matrix), heatmap

What changed as far as I can tell:

• heatmap no longer errors for reverse intervals but otherwise works the same (is the RangeLike convert not getting hit?)
• image no longer errors on reverse intervals and allows for reversing, which it previously didn't. This is visually different
• PointBased no longer errors with (Interval, Interval, Matrix), but still with something like (Interval, Vector)
• volume errors in data_limits instead of convert_arguments with reversed limits

[aplavin]

Bump :)

[aplavin]

Any issues with this?.. Or suggestions on how to proceed with this bugfix?

[aplavin]

another bump...

[SimonDanisch]

Sorry I guess we're all a bit unsure how to best do this, and if this PR is enough and whether we have enough tests.
I think this also changed recently with my heatmap fastpath PR - in the backends it should now always be Makie.EndPoints for x,y that have just a beginning and end.
Lets try to resolve the conflict and merge this soon one way or another.

[aplavin]

Hmm, the new behavior for reversed axes seems worse: earlier, it threw an error, but now displays the exact same plot as non-reversed:

This leads to a mismatch between coordinates and actual values, and can potentially be highly misleading.

Trying to understand EndPoints and its handling... I wonder what was the issue with simply using an interval instead?

Makie.jl/MakieCore/src/types.jl

Line 154 in 8617742

# But ClosedInterval doesn't support all operations/constructions we need

Best to make a new PR on top of the current master I guess.

[SimonDanisch]

Maybe I started using minmax, which is incorrect here, isn't it?