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

Resolved: Heatmap transposes matrices #205

Closed
mkborregaard opened this issue Nov 1, 2018 · 36 comments
Closed

Resolved: Heatmap transposes matrices #205

mkborregaard opened this issue Nov 1, 2018 · 36 comments

Comments

@mkborregaard
Copy link
Contributor

mkborregaard commented Nov 1, 2018

I'm updating the worldclim example to 1.0 and fixing it, and ran into this issue:

Makie.heatmap(water[10])

skaermbillede 2018-11-01 kl 11 29 02

Plots.heatmap(water[10])

skaermbillede 2018-11-01 kl 11 28 32

(note that the matrix format in the example is a bit funky in both examples).

I have a strong feeling this is the source of the issues described in #137

@SimonDanisch
Copy link
Member

Hm, that should be unrelated! I thought on master I made sure, that the values from the axis map to the right pixels... let me check!

@SimonDanisch
Copy link
Member

(unrelated, because putting it on a mesh could yet have another messed up mapping -.-)

@mkborregaard
Copy link
Contributor Author

This is on master btw

@SimonDanisch
Copy link
Member

for

x = -0.1:0.1:4
y = range(-2, stop=6, length = length(x))
z = x .* y'
Makie.heatmap(x, y, z)

image
Makie is right, no?

@SimonDanisch
Copy link
Member

SimonDanisch commented Nov 1, 2018

brightest needs to be 4x*6y, and darkest 4x*-2y

@mkborregaard
Copy link
Contributor Author

waitaminute - is Plots heatmap broken then? :-O

@mkborregaard
Copy link
Contributor Author

mkborregaard commented Nov 1, 2018

Ah, no @SimonDanisch the x axis is the horizontal one, but you're using y as horizontal in your calculation. it should be z = x' .* y not z = x .* y'. And then you get

skaermbillede 2018-11-01 kl 21 02 56

@SimonDanisch
Copy link
Member

oh, subtle^^ guess i need to change the orientation yet another time :-D

@daschw
Copy link
Contributor

daschw commented Nov 1, 2018

I'm not sure but I think it's debatable, whether the x axis should correspond to the rows (Makie) or columns (Plots) of a matrix. Here's the behaviour for a simple matrix in different plotting packages:

julia> mat = [1 0; 2 0; 3 0]
3×2 Array{Int64,2}:
 1  0
 2  0
 3  0
julia> heatmap(mat)

Makie
makie_hm

Plots
plotshm

PlotlyJS matches the Makie behaviour:

julia> heatmap(z = mat)

plotlyjs_hm

Python's Seaborn e.g. looks like this:

import numpy as np
import seaborn as sns
mat = np.matrix('1 0; 2 0; 3 0')
sns.heatmap(mat)

output

which is the same as Matplotlib's imshow:

import matplotlib.pyplot as plt
plt.imshow(mat)

mpl

I think both, Makie/PlotlyJS and and Matplotlib/Seaborn, make sense in their way:

  • MPL/Seaborn show the image like the matrix would be written/printed (This however flips the y axis)
  • Makie/PlotlyJS match the "first" (row) dimension with the "first" (x) axis. However, this makes it harder in my opinion to make the mental link between written/printed matrix and image.

I'm not sure if there is a "correct" way to do this, but I think the hybrid Plots implementation (match x axis with columns, but avoid y axis flipping) is not as clear and less preferable. What do you say?

@daschw
Copy link
Contributor

daschw commented Nov 1, 2018

There's definitely something wrong with the Makie example, though, considering the axes tick labels.

@mkborregaard
Copy link
Contributor Author

mkborregaard commented Nov 1, 2018

I disagree. The call should be x, y, z, not e.g. y, x, z for the heatmap case in particular. The axes of all plots are x on the bottom and increasing towards the right, y going upwards, and IMHO it is confusing to make that different for this one particular plot call.

It also would make it much more difficult to e.g. plot scatter plot points / contours on top of a heatmap if the axes were non-traditional here (something I use all the time, e.g. for maps).

Also, Plots' implementation is not some hybrid implementation, it's the default in GR, in R and a number of other plotting packages.

@daschw
Copy link
Contributor

daschw commented Nov 1, 2018

It also would make it much more difficult to e.g. plot scatter plot points / contours on top of a heatmap if the axes were non-traditional here (something I use all the time, e.g. for maps).

Very good point.

@daschw
Copy link
Contributor

daschw commented Nov 1, 2018

I agree with you. The Plots implementation does make sense and heatmap should not behave any differently than contour here.

@mkborregaard
Copy link
Contributor Author

mkborregaard commented Nov 2, 2018

@SimonDanisch , @daschw
I know I argued very strongly above. But actually I've reflected further on this, and I have to backpedal. I think you're right.

I still think x must correspond to x and y correspond to y, and values of x and y increase to the right and upwards. But that is also the case in the current Makie / PlotlyJS implementation. It's just a question of how you define the relationship.

Looking at Simon's and my example above:

x = -0.1:0.1:4
y = range(-2, stop=6, length = length(x))
z_makie = x .* y'
z_plots = x' .* y
z_makie[20, 10] == x[20] * y[10]
# true
z_plots[20, 10] == x[20] * y[10]
# false

I know this example doesn't prove anything, but just shows that the Makie interpretation is as intuitive as my first one, that x should be horizontal.

Also, I had to go back and check the claims I made about GR and Makie, and they are false.

R does the same as PlotlyJS/Makie:
skaermbillede 2018-11-02 kl 08 15 27

While GR lives in a world of it's own:
skaermbillede 2018-11-02 kl 08 15 49

Matlab, FWIW, does what matplotlib does.

So, my conclusion is that I've been suffering from Plots' Stockholm syndrome and not thinking this all the way through. And that we should close this issue and keep the Makie implementation as it is.

The second question then is - should we change Plots to align with Makie?

@mschauer
Copy link
Contributor

mschauer commented Nov 2, 2018

Hm, Makie has one transpose and one axis flip compared to the terminal representation of a matrix:

[1  0
 2  0
 3  0]

vs

[ 0 0 0
  1 2 3]

so all other things equal, if we can get closer to the terminal representation that would be good. The y-axis flip is imho best justified by the axis-direction argument.

@mkborregaard
Copy link
Contributor Author

The correspondence to terminal representation is what controls the differences between matplotlib(seaborn)/matlab on the one hand, and makie/plotlyjs/R on the other. But that flip IS necessary in order for allowing adding other plots to the scene in the same coordinate system, and that is crucial IMHO.

@Evizero
Copy link
Contributor

Evizero commented Nov 2, 2018

note that an image in julia should basically be displayed the same as a matrix in the terminal.

img

While a heatmap is not really the same thing as a pixel array, I tend to think of it as somewhat similar. For example if I want to visualize a matrix of numbers I'd like to say "imagify this matrix using some nice colormapping". The transpose throws me off a little.

then again, the "x, y, z" instead of "y, x, z" argument is also pretty convincing.

@mkborregaard
Copy link
Contributor Author

mkborregaard commented Nov 2, 2018

FWIW I now think of the Plots' and Makie' versions as equally good (there are advantages with both), with the Makie having the advantage(?) of being shared by other plotting packages.
I think the terminal-array vs plotting-coordinate debate relates a bit to an evolution in the way people tend to think about plots.
For just a heatmap it makes sense to try to be close to the input types, so in old-fashioned plotting (like matlab) where a heatmap is a standalone thing it makes sense to be close to a Matrix show.
But in modern plotting people tend to think of a heatmap as just a plottype/seriestype/geom/whatever, and that input data are just input data - i.e. a heatmap is a type of plot, not a way of showing a matrix. That greatly simplifies composing plots, which is why newer plotting systems such as plotly tends to use the plot coordinate system.

@Evizero
Copy link
Contributor

Evizero commented Nov 2, 2018

That argument makes sense to me. I think as long as its reasonably simple to achieve the "matrix-showing" usecase (be it by using a setting or by transposing the data) everyone is served

@daschw
Copy link
Contributor

daschw commented Nov 2, 2018

Can't we have both, a heatmap, that behaves like Makie currently does (axes ticks need to be fixed though) and sticks to the "y axis increasing from bottom to top" - convention for easy composability with scatter, contour etc. and something like imshow / spy to quickly view matrices as they would be printed in a terminal? It's just a matter of implementing different recipes, right?

@mschauer
Copy link
Contributor

mschauer commented Nov 2, 2018

I might also switch sides: in plotting a vector, going from x[i] to x[i+1] corresponds to a change in the horizontal axis to the right.

@daschw
Copy link
Contributor

daschw commented Nov 2, 2018

in plotting a vector, going from x[i] to x[i+1] corresponds to a change in the horizontal axis to the right.

That's a very good, intuitive way to look at it.

We agree then that the current Makie behaviour makes sense? I can't believe that Michael made me change my mind twice 😀

So, should we also change this in Plots? That would really be a breaking change.

@mkborregaard
Copy link
Contributor Author

Well @daschw that's because you made me change my mind so we're even :-) And I agree with @Evizero that there should be a userfacing keyword to transpose the matrix, as well as of course a y/xflip argument, so discerning users can customize. And with @daschw (again) that there should be a spy recipe specifically for showing matrices.

Finally, I'd be fine with either or, changing Plots or leaving as is. The path of least resistance is to leave as is, we just need to keep this in mind when porting recipes.

@daschw
Copy link
Contributor

daschw commented Nov 2, 2018

Least resistance sounds good.

@mkborregaard mkborregaard changed the title Heatmap transposes matrices Resolved: Heatmap transposes matrices Nov 2, 2018
@mkborregaard
Copy link
Contributor Author

Right - let's keep it open as a reminder to implement the transpose/flip

@SimonDanisch
Copy link
Member

thanks for the detective work here :)
I'm glad that my intuitive understanding of this wasn't completely out of the world!
I'd love to default to show heatmaps like images, so that images by default show correctly, but when I realized that this would need to either flip the axis or disregard the value mapping from x -> matrix index, I also came to the conclusion that it makes sense to get away from that in a plotting library!
Either having a simple kw arg or imshow would be great to recover this! I'd prefer a generic kw arg to offer this for other plot types as well - maybe with a imshow recipe utilizing that attribute!

@rafaqz
Copy link
Contributor

rafaqz commented Nov 14, 2018

This is a more general issue than heatmaps in plotting packages, it seems to come up anywhere a matrix is converted to an image.

A matrix converted to an image with Images.jl and in Cairo/Gtk show rotated and as in the REPL respectively. Images.jl and ArchGDAL.jl disagree on which way to import images to matrix from a raster file. It might be good to have a general discussion across Julia packages for this sometime...

@ssfrr
Copy link
Contributor

ssfrr commented Nov 26, 2018

Notice that the axes in @daschw's makie plot above are not correct - the heatmap is 2 blocks wide and 3 blocks tall, but the axes are showing 3 blocks wide and 2 blocks tall.

edit: whoops, just noticed that @daschw brought up the same issue with the axes ticks

@tshort
Copy link

tshort commented Nov 27, 2018

I just ran into the problem that the axis scales are flipped. In @daschw's plot, the X axis should go from 0 to 3, and the Y axis should go from 0 to 2.

@tshort
Copy link

tshort commented Nov 27, 2018

If you swap m and n in the following line, it fixes the issue with swapped axes scales.

https://github.com/JuliaPlots/AbstractPlotting.jl/blob/99ebdbb1903e72e2dc5e6a4d7b0a1112819d4cd7/src/conversions.jl#L182

I'm not sure that's the right fix, though. (I don't follow the code very well.)

@ssfrr
Copy link
Contributor

ssfrr commented Nov 27, 2018

That seems like a reasonable fix - the idea is that we want to interpret the size(matrix) as (x,y), so doing it as the point where the matrix is interpreted as data makes sense to me.

@SimonDanisch
Copy link
Member

I think all heatmap issues should be fixed!

@jerlich
Copy link

jerlich commented Feb 22, 2019

Sorry, I'm confused. Is there an upstream fix (in AbstractPlotting) that we are waiting for? The axes are still flipped relative to the image in master.

@mkborregaard
Copy link
Contributor Author

Not to sound snarky, but I think you would not be confused if you read the whole issue.

@ssfrr
Copy link
Contributor

ssfrr commented Feb 22, 2019

The problem he brings up in #295 is still present. It's the same one brought up here, here, and here. It's not a question of whether the whole plot should be transposed or not, but that the data and the axes need to be consistent.

I think that @tshort has suggested a reasonable fix.

@c42f
Copy link
Contributor

c42f commented Mar 18, 2019

FWIW, I very much like the way that contour and heatmap have turned out, with heatmap(x,y,z) doing exactly what I expect. IMO this ordering is the only one which is systematic and consistent. That is, the following orderings all match:

  • The written order of function arguments heatmap(coord1, coord2, data)
  • The written order of indices data[index1,index2]
  • The alphabetical order of conventional axis names axis1=x, axis2=y

However we still have the fact that images appear rotated when naively loaded from file using FileIO.load() and displayed with image which seems less than ideal.

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

No branches or pull requests

10 participants