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

add default colorscale attribute #2493

Closed
wants to merge 40 commits into from
Closed

Conversation

t-bltg
Copy link
Collaborator

@t-bltg t-bltg commented Dec 13, 2022

Description

Partially fix #1405.

First example

old approach
using CairoMakie

x = 10.0.^(1:0.1:4)
y = 1.0:0.1:5.0
z = broadcast((x, y) -> log10(x), x, y')  # NOTE: the scaling is also applied here

axis = (; scale = log10, minorticksvisible = true, minorticks = IntervalsBetween(9))

fig, ax, hm = heatmap(x, y, z; axis = NamedTuple(Symbol(:x, k) => v for (k, v) in pairs(axis)))
Colorbar(fig[1, 2], hm; minorticksvisible = true, minorticks = IntervalsBetween(9)

Makie.save("log_cbar.png", fig)

log_cbar

test CairoMakie passes locally.

I also believe that the user shouldn't be responsible for scaling height values (i.e. using (x, y) -> x should be scaled by xscale and yscale, maybe as suggested by adding a cscale or zscale for 2D Axis, but this is left for another issue / discussion).

Unscale the Colorbar axis limits, such that the following yields a correctly scaled colorbar:

Add a colorscale attribute, and make colorbar inherit this attribute by default. The rationale is that the colorbar colors are linked to the plot colors (or z values, heights). Consequently, only setting the scale attribute of the colorbar when creating it is not sufficient to correctly render scaled colors. This PR makes Makie consistent with other libraries such as gnuplot or pyplot.

new approach

using CairoMakie

x = 10.0.^(1:0.1:4);
y = 1.0:0.1:5.0;
z = broadcast((x, y) -> x, x, y');  # NOTE: don't apply the transformation here

axis = (; xscale = log10, xminorticksvisible = true, xminorticks = IntervalsBetween(9));

fig, ax, hm = heatmap(x, y, z; colorscale = log10, axis);  # NOTE: added `colorscale` attr
Colorbar(fig[1, 2], hm; minorticksvisible = true, minorticks = IntervalsBetween(9));  # inherits `hm.colorscale` by default

display(fig)
Makie.save("log_cbar.png", fig);

log_cbar

Python version

import PythonPlot

main() = begin
  x = 10.0.^(1:0.1:4)
  y = 1.0:0.1:5.0
  z = broadcast((x, y) -> x, x, y')
  fig, ax = PythonPlot.subplots()
  norm = PythonPlot.matplotlib.colors.LogNorm()
  cax = ax.pcolor(x, y, z'; cmap="viridis", norm)
  ax.set_xscale("log"; base=10)
  cb = fig.colorbar(cax; norm)
  cb.ax.minorticks_on()
  fig.savefig("log_cbar_py.png")
  fig
end

main()

log_cbar_py

Second example

I'm going to rework this PR using the example from @lazarusA on discourse:

using CairoMakie

main() = begin
  x = [1925, 10620, 13434, 46577, 7615, 21049, 17344, 7763, 44056, 44401, 16986, 22818, 44138, 3161, 12984, 17415, 41240, 8501, 1830, 7983, 6295, 9833, 17196, 15441, 73003, 16371, 1654, 777, 3267, 2897, 43294, 6514, 599, 2191, 22465, 13334, 12761, 1472, 809, 6220, 14132, 3491, 20260, 21291, 29797, 29437, 43495, 3139, 10503, 12837, 10996, 11031, 7776, 31087, 1129, 26812, 1520, 7925, 38923, 37599, 18627, 1644, 7474, 44053, 4099, 25430, 11593, 7279, 1225, 1386, 6816, 1710, 4270, 24200, 42182, 5903, 10504, 15573, 14646, 47758, 31590, 33297, 8606, 36162, 11752, 23468, 2898, 1824, 82633, 3245, 5212, 23282, 17050, 2598, 958, 17261, 26665, 88314, 12547, 1400, 799, 24320, 14408, 1684, 30265, 3661, 3877, 18350, 16850, 3510, 4896, 11819, 14833, 7319, 1176, 4012, 10040, 2352, 45784, 34186, 4712, 943, 5727, 1390, 64304, 48226, 4743, 20485, 2529, 8219, 11903, 6876, 24787, 26437, 132877, 19203, 23038, 1549, 5558, 3003, 52469, 2251, 12908, 25684, 2085, 80794, 27204, 28550, 2047, 624, 12509, 34644, 3047, 32979, 10624, 9997, 10435, 3975, 17125, 6095, 44892, 56118, 4637, 2582, 2571, 14512, 2086, 1433, 5069, 30113, 11126, 19360, 15865, 1680, 8449, 60749, 38225, 53354, 20438, 5598, 2912, 15753, 5623, 4319, 3887, 4034, 1801]
  y = [57.63, 76.0, 76.5, 84.1, 61.0, 75.2, 76.2, 74.4, 81.8, 81.0, 72.9, 72.3, 79.2, 70.1, 75.8, 70.4, 80.4, 70.0, 65.5, 70.2, 72.3, 77.9, 66.4, 75.6, 78.7, 74.9, 62.8, 60.4, 68.4, 59.5, 81.7, 74.6, 53.8, 57.7, 79.3, 76.9, 75.8, 64.1, 58.3, 61.9, 80.0, 60.33, 78.0, 78.5, 82.6, 78.6, 80.1, 64.63, 74.6, 73.8, 75.2, 71.3, 74.1, 60.63, 62.9, 76.8, 63.6, 66.3, 80.8, 81.9, 60.53, 65.1, 73.3, 81.1, 65.5, 79.8, 71.7, 73.1, 60.8, 53.4, 64.4, 65.3, 72.4, 76.2, 82.8, 66.8, 70.9, 78.5, 72.1, 80.4, 82.4, 82.1, 75.5, 83.5, 78.3, 68.2, 66.63, 62.4, 80.7, 69.0, 66.4, 75.7, 78.5, 48.5, 63.9, 76.2, 75.4, 81.1, 77.0, 64.7, 60.22, 75.1, 79.5, 57.6, 82.1, 65.1, 65.7, 73.9, 74.5, 67.0, 72.7, 65.3, 75.8, 74.7, 56.4, 67.9, 61.0, 71.2, 80.6, 80.6, 76.8, 62.2, 61.33, 71.4, 81.6, 75.7, 66.5, 78.2, 60.6, 73.9, 77.5, 70.2, 77.3, 79.8, 82.0, 76.8, 73.13, 66.53, 72.2, 68.8, 78.1, 66.1, 78.1, 73.7, 58.5, 82.1, 76.4, 80.2, 64.1, 58.7, 63.72, 80.7, 58.0, 81.7, 76.5, 74.5, 72.9, 69.5, 70.5, 51.5, 82.0, 82.9, 70.26, 71.0, 63.43, 75.1, 72.4, 64.23, 70.5, 71.4, 77.3, 76.5, 67.9, 60.8, 72.1, 76.6, 81.4, 79.1, 77.3, 70.1, 65.0, 75.8, 76.5, 75.2, 67.6, 58.96, 60.01]
  popsize = [32526562, 2896679, 39666519, 70473, 25021974, 91818, 43416755, 3017712, 23968973, 8544586, 9753968, 388019, 1377237, 160995642, 284215, 9495826, 11299192, 359287, 10879829, 774830, 10724705, 3810416, 2262485, 207847528, 423188, 7149787, 18105570, 11178921, 15577899, 23344179, 35939927, 520502, 4900274, 14037472, 17948141, 1376048943, 48228704, 788474, 77266814, 4620330, 4807850, 22701556, 4240317, 11389562, 1165300, 10543186, 5669081, 887861, 72680, 10528391, 16144363, 91508084, 6126583, 845060, 5227791, 1312558, 99390750, 892145, 5503457, 64395345, 1725292, 1990924, 3999812, 80688545, 27409893, 10954617, 106825, 16342897, 12608590, 1844325, 767085, 10711067, 8075060, 9855023, 329425, 1311050527, 257563815, 79109272, 36423395, 4688465, 8064036, 59797685, 2793335, 126573481, 7594547, 17625226, 46050302, 112423, 3892115, 5939962, 6802023, 1970503, 5850743, 2135022, 4503438, 6278438, 2878405, 567110, 2078453, 24235390, 17215232, 30331007, 363657, 17599694, 418670, 52993, 4067564, 1273212, 127017224, 104460, 4068897, 2959134, 625781, 34377511, 27977863, 53897154, 2458830, 28513700, 16924929,4528526, 6082032, 19899120, 182201962, 25155317, 5210967, 4490541, 188924874, 3929141, 7619321, 6639123, 31376670, 100699395, 38611794, 10349803, 2235355, 19511324, 143456918, 11609666, 193228, 190344, 31540372, 15129273, 8850975, 96471, 6453184, 5603740, 5426258, 2067526, 583591, 10787104, 54490406, 50293439, 12339812, 46121699, 20715010, 184999, 109462, 40234882, 542975, 1286970, 9779426, 8298663, 18502413, 8481855, 53470420, 67959359, 1184765, 7304578, 106170, 1360088, 11253554, 78665830, 5373502, 39032383, 44823765, 9156963, 64715810, 321773631, 3431555, 29893488, 264652, 31108083, 93447601, 4668466, 26832215, 16211767, 15602751]
  population = [32526562, 2896679, 39666519, 70473, 25021974, 91818, 43416755, 3017712, 23968973, 8544586, 9753968, 388019, 1377237, 160995642, 284215, 9495826, 11299192, 359287, 10879829, 774830, 10724705, 3810416, 2262485, 207847528, 423188, 7149787, 18105570, 11178921, 15577899, 23344179, 35939927, 520502, 4900274, 14037472, 17948141, 1376048943, 48228704, 788474, 77266814, 4620330, 4807850, 22701556, 4240317, 11389562, 1165300, 10543186, 5669081, 887861, 72680, 10528391, 16144363, 91508084, 6126583, 845060, 5227791, 1312558, 99390750, 892145, 5503457, 64395345, 1725292, 1990924, 3999812, 80688545, 27409893, 10954617, 106825, 16342897, 12608590, 1844325, 767085, 10711067, 8075060, 9855023, 329425, 1311050527, 257563815, 79109272, 36423395, 4688465, 8064036, 59797685, 2793335, 126573481, 7594547, 17625226, 46050302, 112423, 3892115, 5939962, 6802023, 1970503, 5850743, 2135022, 4503438, 6278438, 2878405, 567110, 2078453, 24235390, 17215232, 30331007, 363657, 17599694, 418670, 52993, 4067564, 1273212, 127017224, 104460, 4068897, 2959134, 625781, 34377511, 27977863, 53897154, 2458830, 28513700, 16924929,4528526, 6082032, 19899120, 182201962, 25155317, 5210967, 4490541, 188924874, 3929141, 7619321, 6639123, 31376670, 100699395, 38611794, 10349803, 2235355, 19511324, 143456918, 11609666, 193228, 190344, 31540372, 15129273, 8850975, 96471, 6453184, 5603740, 5426258, 2067526, 583591, 10787104, 54490406, 50293439, 12339812, 46121699, 20715010, 184999, 109462, 40234882, 542975, 1286970, 9779426, 8298663, 18502413, 8481855, 53470420, 67959359, 1184765, 7304578, 106170, 1360088, 11253554, 78665830, 5373502, 39032383, 44823765, 9156963, 64715810, 321773631, 3431555, 29893488, 264652, 31108083, 93447601, 4668466, 26832215, 16211767, 15602751]
  country = ["Afghanistan", "Albania", "Algeria", "Andorra", "Angola", "Antigua and Barbuda", "Argentina", "Armenia", "Australia", "Austria", "Azerbaijan", "Bahamas", "Bahrain", "Bangladesh", "Barbados", "Belarus", "Belgium", "Belize", "Benin", "Bhutan", "Bolivia", "Bosnia and Herzegovina", "Botswana", "Brazil", "Brunei", "Bulgaria", "Burkina Faso", "Burundi", "Cambodia", "Cameroon", "Canada", "Cape Verde", "Central African Republic", "Chad", "Chile", "China", "Colombia", "Comoros", "Congo, Dem. Rep.", "Congo, Rep.", "Costa Rica", "Cote d'Ivoire", "Croatia", "Cuba", "Cyprus", "Czech Republic", "Denmark", "Djibouti", "Dominica", "Dominican Republic", "Ecuador", "Egypt", "El Salvador", "Equatorial Guinea", "Eritrea", "Estonia", "Ethiopia", "Fiji", "Finland", "France", "Gabon", "Gambia", "Georgia", "Germany", "Ghana", "Greece", "Grenada", "Guatemala", "Guinea", "Guinea-Bissau", "Guyana", "Haiti", "Honduras", "Hungary", "Iceland", "India", "Indonesia", "Iran", "Iraq", "Ireland", "Israel", "Italy", "Jamaica", "Japan", "Jordan", "Kazakhstan", "Kenya", "Kiribati", "Kuwait", "Kyrgyz Republic", "Lao", "Latvia", "Lebanon", "Lesotho", "Liberia", "Libya", "Lithuania", "Luxembourg", "Macedonia, FYR", "Madagascar", "Malawi", "Malaysia", "Maldives", "Mali", "Malta", "Marshall Islands", "Mauritania", "Mauritius", "Mexico", "Micronesia, Fed. Sts.", "Moldova", "Mongolia", "Montenegro", "Morocco", "Mozambique", "Myanmar", "Namibia", "Nepal", "Netherlands", "New Zealand", "Nicaragua", "Niger", "Nigeria", "North Korea", "Norway", "Oman", "Pakistan", "Panama", "Papua New Guinea", "Paraguay", "Peru", "Philippines", "Poland", "Portugal", "Qatar", "Romania", "Russia", "Rwanda", "Samoa", "Sao Tome and Principe", "Saudi Arabia", "Senegal", "Serbia", "Seychelles", "Sierra Leone", "Singapore", "Slovak Republic", "Slovenia", "Solomon Islands", "Somalia", "South Africa", "South Korea", "South Sudan", "Spain", "Sri Lanka", "St. Lucia", "St. Vincent and the Grenadines", "Sudan", "Suriname", "Swaziland", "Sweden", "Switzerland", "Syria", "Tajikistan", "Tanzania", "Thailand", "Timor-Leste", "Togo", "Tonga", "Trinidad and Tobago", "Tunisia", "Turkey", "Turkmenistan", "Uganda", "Ukraine", "United Arab Emirates", "United Kingdom", "United States", "Uruguay", "Uzbekistan", "Vanuatu", "Venezuela", "Vietnam", "West Bank and Gaza", "Yemen", "Zambia", "Zimbabwe"]

  markersize = 20(popsize ./ maximum(popsize)).^(0.4)
  color = popsize ./ 1e6

  fig, ax, pl = scatter(
    x, y;
    markersize,
    color,
    colormap=:plasma,
    colorrange=(10^-1, 10^3),
    colorscale=log10,  # NOTE: added `colorscale` attr
    axis=(;
      title="gapminder",
      xlabel="income",
      ylabel="health",
      xscale=log10,
      xminorticksvisible=true,
      xminorticks=IntervalsBetween(9),
    )
  )

  for i  eachindex(x)
    population[i] > 2.5e8 && text!(ax, country[i]; position=(x[i], y[i] - 1))
  end

  Colorbar(
    fig[1, 2], pl;
    label="population x million",
    minorticksvisible=true,
    minorticks=IntervalsBetween(9),
  )

  fig
end

main()

income

TODO:

  • fix GLMakie, WGLMakie transforms

Type of change

Delete options that do not apply:

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

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@t-bltg t-bltg force-pushed the log_cbar branch 2 times, most recently from 080dfd7 to 21a0966 Compare December 13, 2022 15:49
@t-bltg t-bltg marked this pull request as draft December 22, 2022 12:23
@t-bltg t-bltg marked this pull request as ready for review December 22, 2022 16:31
@t-bltg t-bltg changed the title fix Colorbar scale fix image color sampling when applying axis scale Dec 22, 2022
@t-bltg t-bltg force-pushed the log_cbar branch 2 times, most recently from 6ac03c8 to 1f08f95 Compare December 22, 2022 16:57
@t-bltg t-bltg changed the title fix image color sampling when applying axis scale fix image color sampling on axis scale Dec 22, 2022
@lazarusA
Copy link
Contributor

Thanks for taking a look at this again. It looks great already. I can help with more examples just to be sure, the more the merrier.
The following is a nice looking one. Is simple but nice. Something is completely off, not sure if is the data or Makie.heatmap!

using GLMakie
using Random: seed!
seed!(123)
x = y = 1:100
z = rand(1:100, 100,100)

fig = Figure()
ax = Axis(fig[1,1], aspect = 1, xscale = log10, yscale = log10)
hm = heatmap!(ax, x, y, z; colormap = :gist_stern)
Colorbar(fig[1,2], hm, scale = log10)
fig

logsMakie

Expected output:

using Gnuplot
@gp x y z "w image pixels notit" "set auto fix" "set size square" :-
@gp :- yrange = (0.5,100) :-
@gp :- "set logscale x" "set logscale y" "set logscale cb" :-
@gp :- xlab = "x" ylab = "y" "set cblabel 'z'" palette(:gist_stern)
Gnuplot.save(term="pngcairo size 600,400", output="logs_gnuplot.png")

logs_gnuplot

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 22, 2022

Thanks for the feedback.

Yes, before I work further on this PR, I want to make sure that the approach is also valid with @SimonDanisch and @jkrumbiegel.

I've only modified CairoMakie for now, but not yet WGLMakie or GLMakie (since modifying low level GPU stuff such as fragment shaders would be more difficult I guess ...)

Here is what I obtain for your example with the CairoMakie backend:

using CairoMakie
import Random
Random.seed!(123)
x = y = 1:100;
z = rand(2:100, 100, 100);   # NOTE: starting at 2, to avoid undefined (log10 ∘ log10)(1), I think we should add a `colorscale` argument instead !
fig = Figure();
ax = Axis(fig[1, 1], aspect=1, xscale=log10, yscale=log10);
hm = heatmap!(ax, x, y, z; colormap=:gist_stern);
Colorbar(fig[1, 2], hm, scale=log10);
display(fig)
Makie.save("stern.png", fig)

stern

On second thought I think we should add a cscale or colorscale argument to heatmap instead of composing transformations.
This will allow default color scaling and inherited colorbar scaling.

With a colorscale=log10 argument:

using CairoMakie
import Random
Random.seed!(123)
x = y = 1:100;
z = rand(1:100, 100, 100);
fig = Figure();
ax = Axis(fig[1, 1], aspect=1, xscale=log10, yscale=log10);
hm = heatmap!(ax, x, y, z; colormap=:gist_stern, colorscale=log10);  # NOTE: added `colorscale` attr
Colorbar(fig[1, 2], hm);  # inherits `scale = p.colorscale` by default
display(fig)
Makie.save("stern.png", fig)
fig

stern

And gnuplot:
logs_gnuplot

@t-bltg t-bltg changed the title fix image color sampling on axis scale add colorscale argument Dec 22, 2022
@t-bltg t-bltg force-pushed the log_cbar branch 3 times, most recently from 6c499ea to 0b4c020 Compare December 22, 2022 22:55
@t-bltg t-bltg changed the title add colorscale argument add default colorscale attribute Dec 22, 2022
@t-bltg t-bltg force-pushed the log_cbar branch 3 times, most recently from 9e0c9fe to 755fc2b Compare December 22, 2022 23:14
@jkrumbiegel
Copy link
Member

Yes I think this one needs to be planned well so that it's feasible for all backends. For example, the transformations on points are all done on the CPU but I'm not sure if we could leverage the GPU for the color transforms, and how that would change the approach.

@lazarusA
Copy link
Contributor

The colorscale keyword arg is a good approach, rather than going into the Colorbar which doesn't feel so intuitive. At the end of day, users like me just want something that works first and sometimes planning for too long stalls important PRs. I like @t-bltg approach of going for it.

@jkrumbiegel
Copy link
Member

As it needs to work on all backends, at least some planning or thinking about it seems necessary. I can't really offer a glmakie perspective, from a user perspective I think passing a colorscale attribute seems reasonable. Although I never really liked how color is handled via multiple separate attributes that sometimes go together and sometimes not. This also makes it such that you cannot use colormaps with strokes on scatter, because that stuff is only defined for the fill. But isn't that separation pretty arbitrary? Other recipes have multiple things to be colored, so the "one colormap, one colorrange" idea breaks down anyway. I would probably prefer putting all this into one object but not sure how that would play with updating only colormap or only colorrange like you currently can.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 23, 2022

Thanks both for the comments.

I would probably prefer putting all this into one object but not sure how that would play with updating only colormap or only colorrange like you currently can.

I have also thought about using a mutable Colormap struct with attributes such as map, range, scale, ..., but triggering on this observable or its fields might be less easier ?
It would also be easy to unambiguously parse things such as colormap = (:viridis, (10^-1, 10^3), log10) based on types. We could also consider using the undocumented Makie.sampler for that purpose.
I'm not sure though that adding a layer of complexity would enhance Makie for developers.
The current, being explicit - even if having more and more keywords is less user oriented - is easy to work with and debug, and fast from a pipeline pov.

As it needs to work on all backends, at least some planning or thinking about it seems necessary

Yes, this is why Simon's global view or comments might be helpful here, before I can try updating GLMakie or WGLMakie. Anyway, now that colorscale is propagated to all recipes together with e.g. colormap, this also needs to be tested as well for anything else other than heatmap to see if the drop-in additions are taken into account as expected (I'm e.g. thinking of contour or tricontourf which handle colormaps in special ways).

The colorscale keyword arg is a good approach, rather than going into the Colorbar which doesn't feel so intuitive.

I think scale, colorrange, ... attributes of the Colorbar still needs to be exposed to users for corner cases, or working with multiple colormaps (even if supporting limits or colorrange for the same purpose seems odd to me), but we can work on being as automatic as possible (inherit from plot object) on a simple plot + colorbar case (such as #1405 (comment)).

@t-bltg t-bltg mentioned this pull request Dec 23, 2022
8 tasks
@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 23, 2022

The example adapted from #2496 (comment) with an arbitrary colorscale:

using CairoMakie

himmelblau(x, y) = (x^2 + y - 11)^2 + (x + y^2 - 7)^2
x = y = range(-6, 6; length=100)
z = himmelblau.(x, y')

levels = 10.0.^range(0.3, 3.5; length=10)
f, ax, ct = contour(x, y, z; levels, colormap=:hsv, colorscale=x -> x^0.01)
display(f)

himmelblau

This is exactly the level of control that is suited to these plots.

@lazarusA
Copy link
Contributor

Just in case is relevant, I just noticed that hexbin has already a scale argument,

hexbin(..., scale = log10)

but not sure if the colormap is correct.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 23, 2022

Since scale is applied on count_hex which in turn is used for the colors argument of scatter I think this would be equivalent to using colorscale instead (and would thus make scale obsolete here).

But of course this needs to be carefully checked. ==> done, see below.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 24, 2022

Based on the example in https://docs.makie.org/stable/examples/plotting_functions/hexbin/#changing_the_scale_of_the_number_of_observations_in_a_bin, the following is the output on this branch, replacing scale with colorscale:

using CairoMakie
x = randn(10_000);
y = randn(10_000);
fig = Figure();
hexbin(fig[1, 1], x, y; bins = 40, axis = (aspect = DataAspect(), title = "scale = identity"));
hexbin(fig[1, 2], x, y; bins = 40,  axis = (aspect = DataAspect(), title = "colorscale = log10"), colorscale = log10);
display(fig)
save("hexb.png", fig)

hexb

As a consequence, I'd say that colorscale can be a drop-in replacement for scale for the hexbin recipe (generalization).

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 25, 2022

@SimonDanisch, maybe I assume that this PR is going in the right direction, and that you don't have any objection to the approach ?

Your answer would allow me to start working on adapting GLMakie and WGLMakie (I don't want to invest dev hours in vain).

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Apr 10, 2023

Thanks, will try to get to it next!

@tomchor
Copy link

tomchor commented Apr 15, 2023

Dropping by to voice my support for this PR! I've been waiting eagerly to have this merged! 😂

Please let me know if you want help to get things tested, etc.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Apr 15, 2023

Please let me know if you want help to get things tested, etc.

I'm sure there will be (corner case) bugs, so if there are use cases that we didn't think of (and interactive usages), we'd be glad to hear those ;)

@jkrumbiegel
Copy link
Member

It's looking quite good! Great that you fixed the last bugs I found, sorry to say I got another for you :)

brain = load(assetpath("brain.stl"))
color = [abs(tri[1][2]) for tri in brain for i in 1:3]
fig, ax, m = mesh(brain; color, colorscale=identity)
mesh(fig[1, 2], brain; color, colorscale=log10)
display(fig)
##
m.colorscale = log10

Only when displaying first, then changing the colorscale on m does the output look wrong, the left one should be the same as the right. Interestingly the colorrange values are the same on both, so that's not it.

grafik

@t-bltg
Copy link
Collaborator Author

t-bltg commented Apr 15, 2023

Only when displaying first, then changing the colorscale on m does the output look wrong, the left one should be the same as the right. Interestingly the colorrange values are the same on both, so that's not it.

Fixed: for interactivity in GLMakie, we always have to "listen" on colorscale whenever apply_scale is used.

Question: what does const_lift do ?

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Apr 15, 2023

As a last thing, would you mind changing the new reference tests a little in order to check that the observable updates work. One simple way would be to take the examples where you constructed two plots, one with identity and one with something else, and just make them both identity at first but then change one to log10 or whatever. This way we know that it doesn't merely work at construction, but also changing it after the fact. If changing it after the fact works however, it's pretty much a given that it would have worked at construction (I think I've never had the case that it doesn't, only ever the other way around). If you want, you can of course make three versions to cover all the bases.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Apr 16, 2023

If think the best container for these tests is to use a Stepper for recording frames.
I'm going to put these tests in Makie.jl/ReferenceTests/src/tests/updating.jl.

@jkrumbiegel
Copy link
Member

Ah WGLMakie fails due to a too strictly typed observable, nice that the test caught this :)

@t-bltg
Copy link
Collaborator Author

t-bltg commented Apr 16, 2023

Yeah, I cannot debug WGLMakie at the moment (see the discord question), something changed and I cannot seem to display a WGLMakie plot in my browser anymore..

@t-bltg
Copy link
Collaborator Author

t-bltg commented Apr 16, 2023

@jkrumbiegel, do you have an idea where the attributes are set in WGLMakie, i.e. where the Observable(identity) is created ?

EDIT: lift_convert in WGLMakie/src/serialization.jl

@t-bltg
Copy link
Collaborator Author

t-bltg commented Apr 16, 2023

So I don't really think I can go much further on the WGLMakie bug.
https://github.com/MakieOrg/Makie.jl/blob/master/src/scenes.jl#L185:L186 loses the Observable{Any}(identity) -> Observable(identity), so we cannot change the colorscale obs to e.g. log10, this is because Base functions define their own type since Function is abstract.

I tried to fix lift_convert in WGLMakie without success, and I cannot go on guessing further (since there are no comments) what parts deals with attributes. The lift(convert, plot, value) using wgl_convert is too obscure.

@jkrumbiegel
Copy link
Member

Ok thank you, hopefully @SimonDanisch can take a look at this stage as I'm also not the person to ask about WGLMakie internals. From my side fixing this would be the last blocker to getting this merged, all the other stuff seemed to work pretty well at my level of testing.

@lazarusA
Copy link
Contributor

lazarusA commented Apr 25, 2023

@SimonDanisch please, it will be good not to let this one get behind again. Sorry, I know, too many things.

@davidschlegel
Copy link

Is there any update on this? To me, this PR is quite important.

@SimonDanisch
Copy link
Member

Yes, the backend implementation had a few bugs since every backend has their own way to deal with colormaps and i decided to implement the backend logic more cleanly in #2900.
I will priorise merging this pr in the coming month!

@phyjswang
Copy link

Hi all,

I am wondering this colorscale = log10 feature will be implemented in the near future.
Really need this option 🙏

Thanks!

@SimonDanisch
Copy link
Member

See #2900.

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

Successfully merging this pull request may close these issues.

Heatmap with log scale colorbar (cscale) ?
9 participants