Add Makie contour,contourf, contour! and improve multi-variable plot layout#3088
Add Makie contour,contourf, contour! and improve multi-variable plot layout#3088vincmarks wants to merge 7 commits into
Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3088 +/- ##
==========================================
+ Coverage 96.88% 96.90% +0.02%
==========================================
Files 647 647
Lines 50026 50325 +299
==========================================
+ Hits 48466 48766 +300
+ Misses 1560 1559 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@ranocha, the PR is ready for review |
ranocha
left a comment
There was a problem hiding this comment.
Thanks a lot, looks like this is a nice feature! However, this is also a big PR. I did not review everything, but just have a few first comments. Would it make sense to split this into smaller and independent pieces, e.g., improving the existing layout, Cartesian, and other meshes?
| @timed_testset "Makie contour error handling for 1D solutions" begin | ||
| @test_trixi_include(joinpath(EXAMPLES_DIR, "tree_1d_dgsem", | ||
| "elixir_advection_basic.jl")) | ||
| @test_throws ArgumentError Makie.contour(sol) | ||
| @test_throws ArgumentError Makie.tricontourf(sol) | ||
| end |
There was a problem hiding this comment.
Oops, I forgot to add them. Thanks!
| # Format colorbar tick labels with enough significant figures to distinguish marks, | ||
| # avoiding Makie's default scientific notation | ||
| function _trixi_colorbar_tickformat(values) | ||
| isempty(values) && return String[] | ||
| vmin, vmax = extrema(values) | ||
| range_val = vmax - vmin | ||
| sigfigs = range_val > 0 ? max(3, ceil(Int, -log10(range_val)) + 2) : 4 | ||
| return [string(round(v; sigdigits = sigfigs)) for v in values] | ||
| end | ||
|
|
||
| # Return a non-degenerate (umin, umax) for colorbars; expands zero-width ranges | ||
| # so CairoMakie does not produce NaN when all data values are identical. | ||
| function _trixi_colorbar_limits(umin, umax) | ||
| isapprox(umin, umax) || return (umin, umax) | ||
| delta = max(one(umin), abs(umin)) | ||
| return (umin - delta, umax + delta) | ||
| end | ||
|
|
There was a problem hiding this comment.
Why are these needed, what is wrong with Makie's defaults?
There was a problem hiding this comment.
_trixi_colorbar_tickformat: Makie's default tick formatter switches to scientific notation for small values, which I thought were not nice. And with very narrow value ranges, the default rounding can also make adjacent ticks indistinguishable_trixi_colorbar_limits: This is needed to prevent a crash when all data values are identical (umin == umax). I ran into this withcontourf(pd)onelixir_acoustics_gauss_wall.jl, whererho_mean,v1_mean,v2_mean,c_meanare constant (as shown above in the plots).Makie.tricontourf!crashed with"Can't interpolate in a range where cmin == cmax", and similarly CairoMakie producesNaNwhen constructing a colorbar with a zero-width range.
There was a problem hiding this comment.
This sounds like some of them are workarounds for bugs in Makie.jl. Would you mind filing an issue there or creating a PR to fix the bugs there?
Concerning the tick formatter: Could you please post some example where it changes something? What about cases with values varying over several orders of magnitude?
There was a problem hiding this comment.
Look and feel is always a question of taste, but I like your new version for the first example while I prefer the old version for the second example. Is there a discussion in Makie.jl about the first case, where one should arguably not use the scientific notation?
Yes, I agree, sorry for not following the review checklist guideline of max. 500 lines changed here.
Does that work for you? |
|
Sounds good, thanks! |




This PR addresses the Makie.jl part of #3075
Added/updated
Makie.contourandMakie.contourffor all mesh typesMakie.contour!adds colored isolines on top of an existing plotplot(pd)layoutMaking
plot(pd)prettier:I didn't notice, that for plotting solutions with many variables (e.g

examples/tree_2d_dgsem/elixir_acoustics_gauss.jl) usingplots(pd), plots can currently look like this (Makie.jl)or like this (Plots.jl)

I tried to make this more pretty in something like this (Makie.jl)
Adding
Makie.contour,Makie.contourfandMakie.contour!Here are some visual results