Fix float issues for CairoMakie by using Float64 #2573
Conversation
Compile Times benchmarkNote, 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))
|
|
Very cool, this will enable easy use of DateTime as well, as that can be converted to Float64 losslessly if I'm not wrong |
|
Would it be better to have |
|
Hmm good question. In general, I think we've had issues because of converting things too eagerly. Everything is Float32 because GLMakie needs that eventually, but that has always robbed CairoMakie of using higher precision input data. Although the compilation latency is an argument. I think conversion should generally be the shortest path to a backend-consumable type. |
|
This should fix all the Float32 precision errors in plotting for CairoMakie only. Related issues include: #931, #1579 (error fixed, plotting only in CairoMakie), #396 (is this related?), #1419 (not sure), #780, #1373, #1196 In general this should fix |
src/conversions.jl
Outdated
| # Else, we need to convert it! | ||
| return (GeometryBasics.mesh(mesh, pointtype=Point{N, Float32}, facetype=GLTriangleFace),) | ||
| end | ||
| return (GeometryBasics.mesh(mesh, pointtype=Point{N, Float64}, facetype=GLTriangleFace),) |
There was a problem hiding this comment.
Why did you remove this? I just recently added this to avoid unnecessary conversions, which has been slow, and also materialized any Buffer inside the mesh, breaking this feature: https://docs.makie.org/stable/examples/plotting_functions/mesh/index.html#using_geometrybasicsmesh_and_buffersampler_type
There was a problem hiding this comment.
I just wanted to merge master for now without creating more failures.
There are a ton of probably unnecessary conversion in this, compared to lazy conversions. We'd need a bunch more automatic conversions in GeometryBasics to switch to lazy ones here. I don't really know how to do this correctly, but something like
Base.convert(::Type{<: Rect{N, T}}, r::Rect{N}) where {N, T} = Rect{N, T}(r.origin, r.widths)does the trick of Rect. Maybe that's also something you already worked on somewhere?
| macro trace(expr) | ||
| if expr.head in (Symbol("="), :function) | ||
| name = _to_typed_name(expr.args[1]) | ||
| file = " in " * string(__source__.file) * ':' | ||
| line = string(__source__.line) * '\n' | ||
| code = quote | ||
| begin | ||
| printstyled($name, bold = true) | ||
| printstyled($file, color = :light_black) | ||
| printstyled($line, bold = true) | ||
| $(expr.args[2]) | ||
| end | ||
| end | ||
| out = Expr( | ||
| expr.head, # function or = | ||
| esc(expr.args[1]), # function name w/ args | ||
| esc(code) | ||
| ) | ||
| return out | ||
| end | ||
| return expr | ||
| end | ||
|
|
||
| function _to_typed_name(e::Expr) | ||
| if e.head == :where | ||
| _to_typed_name(e.args[1]) | ||
| elseif e.head == :call | ||
| if length(e.args) > 1 && e.args[2] isa Expr && e.args[2].head == :parameters | ||
| args = join(_to_type.(e.args[3:end]), ", ") | ||
| kwargs = join(_to_type.(e.args[2].args), ", ") | ||
| return string(e.args[1]) * "(" * args * "; " * kwargs * ")" | ||
| else | ||
| args = join(_to_type.(e.args[2:end]), ", ") | ||
| return string(e.args[1]) * "(" * args * ")" | ||
| end | ||
| else | ||
| dump(e) | ||
| return "ERROR" | ||
| end | ||
| end | ||
|
|
||
| _to_type(s::Symbol) = "::Any" | ||
| function _to_type(e::Expr) | ||
| if e.head == Symbol("::") | ||
| return "::" * string(e.args[end]) | ||
| elseif e.head == Symbol("...") | ||
| return _to_type(e.args[1]) * "..." | ||
| elseif e.head == :parameters # keyword args | ||
| return _to_type(e.args[1]) | ||
| elseif e.head == :kw # default value | ||
| return _to_type(e.args[1]) | ||
| else | ||
| dump(e) | ||
| error("Failed to parse expression.") | ||
| return "" | ||
| end | ||
| end |
There was a problem hiding this comment.
I put this together to have an easier time figuring out function call chains. You can put @trace before a function (full or inline) and it'll inject some printing which will print the function header with types, the file and the line number. Should we keep this as a debug tool?
|
I changed things to allow Float32 and Float64 to pass through. Also did some cleanup (write out JuliaGeometry/GeometryBasics.jl#195 was necessary for the pr at some point, not sure if it still is. It would help avoid conversion errors either way though. |
|
Will this also allow to plot data with ranges exceeding the range of Float32 ? |
|
@ffreyer do you have any idea whether this is still "under active development"? This is really a breaking thing for me (I work a lot in loglog scales with data ranges that fall outside Float32 range...). Not pushing you or the Makie community but I would like to make a decision whether to wait for the coolest visualisation tool to get this feature or to go on with Plots.jl... I wish I could help, but I don't think my Julia nor Makie skills are up to par for this... |
Description
This is basically a light version of #2550, aiming to fix float precision errors for CairoMakie but not gl backends. Basically I'm just changing a bunch of variables to Float64 so we don't lose that precision. I will probably go through this again later to remove Float32 -> Float64 conversions.
Some related issues are collected in #2848.
Working
scatter(1:10, 1e9 .+ (1:10))in empty Scene with manual camera.projectionviewvolumeTODO:
Type of change
Checklist
Added or changed relevant sections in the documentation