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

Auto limits broken with recipes on 0.19.7 and 0.19.8 #3176

Closed
3 tasks done
palday opened this issue Aug 24, 2023 · 14 comments · Fixed by #3179
Closed
3 tasks done

Auto limits broken with recipes on 0.19.7 and 0.19.8 #3176

palday opened this issue Aug 24, 2023 · 14 comments · Fixed by #3179
Labels

Comments

@palday
Copy link
Contributor

palday commented Aug 24, 2023

  • are you running newest version (version from docs) ?
  • can you reproduce the bug with a fresh environment ? (]activate --temp; add Makie)
  • What platform + GPU are you on? Linux x86-64, nVidia, but running CairoMakie
version info
julia> versioninfo()
Julia Version 1.9.2
Commit e4ee485e909 (2023-07-05 09:39 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 16 × Intel(R) Xeon(R) E-2288G CPU @ 3.70GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, skylake)
  Threads: 4 on 16 virtual cores
Environment:
  JULIA_EDITOR = code
  JULIA_NUM_THREADS = 4
MWE

Developed when diagnosing a PR in MixedModelsMakie

using CairoMakie, Random

@recipe(CoefPlot, n) do scene
    return Attributes()
end

function Makie.plot!(ax::Axis, P::Type{<:CoefPlot}, allattrs::Makie.Attributes, n)
    plot = Makie.plot!(ax.scene, P, allattrs, n)

    if haskey(allattrs, :title)
        ax.title = allattrs.title[]
    end
    if haskey(allattrs, :xlabel)
        ax.xlabel = allattrs.xlabel[]
    else
        ax.xlabel = "Estimate and 95% confidence interval"
    end
    if haskey(allattrs, :ylabel)
        ax.ylabel = allattrs.ylabel[]
    end
    reset_limits!(ax)
    ax.yticks = 1:n
    ylims!(ax, 0, n + 1)
    return plot
end

function Makie.plot!(plot::CoefPlot{Tuple{Int}})
    n = plot[1][]
    xs = rand(MersenneTwister(42), n)
    ys = collect(1:n)
    se = 0.1
    scatter!(plot, xs, ys)
    errorbars!(plot, xs, ys, se, se; direction=:x)

    return plot
end

f = Figure()
ax = Axis(f[1,1])
coefplot!(ax, 2)
display(f)
Makie 0.19.8

makie0 19 8

Makie 0.19.7

makie0 19 7

Makie 0.19.6

makie0 19 6

Bisected to #8ccb6bdd

Possibly related to #3051

I am unable to replicate this when calling scatter! and errorbars! directly -- it seems to be some interaction with the recipe system.

@jkrumbiegel
Copy link
Member

Hm this must be because the limits for some reason started to include the "whisker" linesegments at the ends of the errorbars, but only if this plot type is not top-level but nested in another recipe. So then it sets the limits way too large

@palday
Copy link
Contributor Author

palday commented Aug 24, 2023

@jkrumbiegel why the asymmetry in the overlarge limits? (xmax is far more inappropriate than xmin)

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Aug 24, 2023

If it does what I think it does, it looks at the space = :pixel values and those would all be positive. Those should just be completely excluded. The bug in the computation was probably always there and just the way errorbars is implemented changed slightly. Is my guess at least. @ffreyer maybe?

@ffreyer
Copy link
Collaborator

ffreyer commented Aug 24, 2023

The pr changed whiskers to be plotted in space = :pixel instead of space = :data, so it's probably the case that they were never ignored and neither is space = :pixel

@palday
Copy link
Contributor Author

palday commented Aug 24, 2023

attempt at a fix in #3179, which does indeed fix the MWE here

@palday
Copy link
Contributor Author

palday commented Aug 28, 2023

@ffreyer FYI I noticed that calling vlines! within a recipe also completely kills the axis limits (haven't yet tested it on current master)

@ffreyer
Copy link
Collaborator

ffreyer commented Aug 28, 2023

In that case h/vspan! probably has issues to.

For h/vlines! the behavior runs through the x/yautolimits attribute which is probably one checked on top level plots in Axis.

@recipe(HLines) do scene
Theme(;
xautolimits = false,
xmin = 0,
xmax = 1,
default_theme(scene, LineSegments)...,
cycle = :color,
)
end

For axis limits to not freak out these plots need to ignore one dimension, rather than the limits of one of the subplots. I'm not sure if there is a way to do this other than the autolimit attributes. Maybe with NaN in the data_limits Rect?

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Aug 29, 2023

not sure if there is a way to do this other than the autolimit attributes

I don't really think so, I did want to change that at some point. Either NaN or maybe Union{Nothing, for each dimension? Because NaN can also happen through bad calculations

@palday
Copy link
Contributor Author

palday commented Sep 10, 2023

Could we go ahead and get a patch release with this fix? It would make teaching this week easier ❤️

@SimonDanisch
Copy link
Member

Sorry, I really wanted to get #3090 into the next release, but I guess I should just tag a new release and make a new one once #3090 is ready.
Release should be ready as soon as the backends are tagged: eda228a

@palday
Copy link
Contributor Author

palday commented Sep 11, 2023

@SimonDanisch thanks!

And yeah, we're not going to run out of release numbers, so release early, release often. 🚀

@palday
Copy link
Contributor Author

palday commented Dec 25, 2023

@SimonDanisch @ffreyer This seems to be back in 0.20.x at least with vlines!

@ffreyer
Copy link
Collaborator

ffreyer commented Dec 25, 2023

0.20.2 had some more fixes for vlines etc. If you're not on 0.20.2+ try updating

@palday
Copy link
Contributor Author

palday commented Dec 25, 2023

On latest (0.20.3). 😢 Notably, this problem now occurs in the plot!(::MyPlotType) method and not the plot!(::Axis, ...) method because I was trying to work around #3514

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

Successfully merging a pull request may close this issue.

4 participants