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

Fix some errors caused by updating Axis xscale or yscale #3084

Merged
merged 19 commits into from
Aug 29, 2023

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Jul 22, 2023

Description

Old Text

This should make the default behavior that transform_func is passed down to child scenes and plots first, before calculations happen.

This fixes #3065, but I haven't tested if it breaks anything else yet.

Side note - Why doesn't map accept priority? And copy, given that it links to the original?

This pr tweaks some behavior around axis scales/transform_funcs to avoid mostly limit related errors.

Changes:

  • Added priority kwarg to our map, map! and lift implementations (Observables is still missing this)
  • Adjusted the order of updates when changing ax.xscale or ax.yscale. This allows barplot to fix its bars before the axis checks them for limits. Basically it makes add smart fillto to enable logscale y-axis histogram barplot #3004 work with dynamic transform_func/scale changes.
    • previously: update limits first, then ax.scene.transformation.transform_func, then axis camera matrices, then plots
    • now: update ax.scene.transformation.transform_func first, then plots, then limits, then axis camera (order enforced by priority)
  • Transformation(parent) now has an overwrite for transform_func
  • hvlines and hvspan no longer back-transform their relative limits to input space. Instead they apply their transform_func to their input data and remove it from their child plots. So rather than plotting to input space they now plot to transformed space. This fixes vlines! throws error when changing yscale from log10 back to identity #3065
  • barplot now runs calculate_bars! at priority 1 to make sure it runs before transform_func is applied in its child plot(s). This shouldn't be necessary, but I left it in for now.
  • errorbar and rangebar changed from using an array of pairs to just a flat array for its linesegments. This avoid a conversion error from Vector to ReinterpretedArray.
  • remove xmin, xmax, ymin, ymax, xautolimits, yautolimits from attribute passthrough in hvlines and hvspan. fixes hvlines bug in GLMakie interactive window #3061

I have tested these changes with

using GLMakie
begin
    fig, ax, li = lines(1:10, 1:10)
    vlines!(ax, 3)
    hlines!(ax, 3)
    bp = barplot!(ax, 1 .+ 5 .* rand(10))
    vspan!(ax, 3, 4)
    hspan!(ax, 3, 4)
    bracket!(ax, 1, 1, 2, 2)
    eb = errorbars!(ax, 1:10, 1:10, [0.3 for _ in 1:10], whiskerwidth = 5)
    text!(ax, Point2f(2), text = "abba")
    tooltip!(ax, Point2f(8), "baab")
    tricontourf!(ax, 1 .+ 4 .* rand(5), 1 .+ 4 .* rand(5), rand(5))
    qqplot!(ax, 5:10, 1:5)
    display(fig)
end
ax.yscale = log10
ax.yscale = identity

Type of change

  • 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.

@MakieBot
Copy link
Collaborator

MakieBot commented Jul 22, 2023

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 12.32s (11.95, 12.81) 0.30+- 1.27s (1.22, 1.31) 0.03+- 755.81ms (717.01, 830.66) 38.02+- 11.12ms (10.71, 11.36) 0.22+- 143.05ms (140.95, 146.47) 2.12+-
master 12.25s (11.96, 12.50) 0.23+- 1.26s (1.22, 1.30) 0.03+- 740.09ms (722.84, 760.62) 15.76+- 11.00ms (10.75, 11.31) 0.21+- 142.52ms (138.48, 144.61) 2.29+-
evaluation +0.52%, 0.06s invariant (0.24d, 0.66p, 0.26std) +0.33%, 0.0s invariant (0.13d, 0.81p, 0.03std) +2.08%, 15.72ms invariant (0.54d, 0.34p, 26.89std) +1.06%, 0.12ms invariant (0.55d, 0.32p, 0.21std) +0.37%, 0.53ms invariant (0.24d, 0.66p, 2.21std)
CairoMakie 11.46s (11.28, 11.71) 0.15+- 1.21s (1.20, 1.24) 0.02+- 227.19ms (222.88, 234.59) 3.79+- 11.35ms (11.19, 11.66) 0.16+- 4.89ms (4.83, 4.96) 0.05+-
master 11.35s (11.05, 11.59) 0.17+- 1.23s (1.17, 1.28) 0.04+- 231.65ms (220.43, 245.98) 8.88+- 11.46ms (11.30, 11.61) 0.11+- 4.89ms (4.79, 4.98) 0.08+-
evaluation +1.00%, 0.11s invariant (0.73d, 0.20p, 0.16std) -1.12%, -0.01s invariant (-0.44d, 0.43p, 0.03std) -1.96%, -4.45ms invariant (-0.65d, 0.26p, 6.34std) -0.96%, -0.11ms invariant (-0.79d, 0.17p, 0.14std) +0.04%, 0.0ms invariant (0.03d, 0.95p, 0.06std)
WGLMakie 12.61s (12.47, 12.73) 0.09+- 1.36s (1.32, 1.40) 0.03+- 11.94s (11.76, 12.13) 0.12+- 13.46ms (12.98, 14.02) 0.44+- 1.10s (1.01, 1.14) 0.04+-
master 12.61s (12.48, 12.93) 0.15+- 1.35s (1.26, 1.38) 0.04+- 11.84s (11.75, 12.01) 0.10+- 13.22ms (12.47, 14.11) 0.65+- 1.09s (1.06, 1.13) 0.03+-
evaluation +0.06%, 0.01s invariant (0.06d, 0.91p, 0.12std) +1.10%, 0.01s invariant (0.45d, 0.42p, 0.03std) +0.82%, 0.1s invariant (0.88d, 0.13p, 0.11std) +1.78%, 0.24ms invariant (0.43d, 0.43p, 0.54std) +1.11%, 0.01s invariant (0.33d, 0.55p, 0.04std)

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jul 23, 2023

barplot! (still?) behaves badly with this. Everything else that directly involves transform_func seems fine to me.

Alternatively to this we could also lower the priority of limit updates in axis etc. Not sure what's better here.

@ffreyer ffreyer changed the title Bump priority of transfrom_func inheritance Fix some errors caused by updating Axis xscale or yscale Jul 25, 2023
@ffreyer ffreyer marked this pull request as ready for review July 25, 2023 21:05
@ffreyer ffreyer requested a review from jkrumbiegel July 27, 2023 11:21
@SimonDanisch
Copy link
Member

@jkrumbiegel @ffreyer can we merge this?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Aug 21, 2023

This was ready on my side. I thought that Julius might have some edge cases in mind related to the changes in the update order in Axis. If not we can go ahead and merge I'd say.

@SimonDanisch SimonDanisch merged commit 5d42257 into master Aug 29, 2023
13 checks passed
@SimonDanisch SimonDanisch deleted the ff/transform_priority branch August 29, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants