Basic Makie.jl support #165
Conversation
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
|
Just saw this really cool update from Julius on the AoG side of things! MakieOrg/AlgebraOfGraphics.jl#619 https://aog.makie.org/v0.9.6/examples/scales/units#units This also looks to address MakieOrg/Makie.jl#3890 in one fell swoop |
|
Cool! Let me know (by tagging me) when you're ready for review |
|
Hi @MilesCranmer, I think this should be just about ready for a first pass of reviews now. The main additions are:
I'm not quite sure yet how I feel about Makie's choice to automatically change non-compound units for folks out from under them, so I kept that bit of machinery out for now. I do see the convenience of it though, it just seems to add a fair bit of complexity that we maybe don't need right now. Instead, this current design favors explicit over implicit unit handling, e.g., Unitful example (taken from ReferenceTests)# Don't swallow units past the first
f, a, p = scatter((1:10) .* u"J/s")
# Don't simplify (assume the user knows better)
scatter(f[1, 2], (1:10) .* u"K", exp.(1:10) .* u"mm/m^2")
# Only change prefixes of simple units, not compound units
scatter(f[2, 1], 10 .^ (1:6) .* u"W/m^2", (1:6) .* 1000 .* u"nm")
# Only change units/prefixes for simple units when adding more plots
scatter(f[2, 2], (0:10) .* u"W/m^2", (0:10) .* u"g")
scatter!((0:10) .* u"kW/m^2", (0:10) .* u"kg")
f
Here the nanometer plot in the bottom left is auto-converted to microns without the user specifying that unit. In contrast: This PRusing DynamicQuantities, Makie
const DQConversion = Base.get_extension(DynamicQuantities, :DynamicQuantitiesMakieExt).DQConversion
fig = Figure()
ax1 = Axis(fig[1, 1]; dim2_conversion=DQConversion(us"J/s"))
ax2 = Axis(fig[1, 2]; dim2_conversion=DQConversion(us"mm/m^2"))
ax3 = Axis(fig[2, 1]; dim1_conversion=DQConversion(us"W/m^2"), dim2_conversion=DQConversion(us"μm"))
ax4 = Axis(fig[2, 2]; dim1_conversion=DQConversion(us"W/m^2"))
scatter!(ax1, (1:10) .* u"J/s")
scatter!(ax2, (1:10) .* u"K", exp.(1:10) .* u"mm/m^2")
scatter!(ax3, 10 .^ (1:6) .* u"W/m^2", (1:6) .* 1000 .* u"nm")
scatter!(ax4, (0:10) .* u"W/m^2", (0:10) .* u"g")
scatter!(ax4, (0:10) .* u"kW/m^2", (0:10) .* u"kg")
figor with the appropriate use of fig = Figure()
scatter(fig[1, 1], (1:10) .* us"J/s")
scatter(fig[1, 2], (1:10) .* u"K", exp.(1:10) .* us"mm/m^2")
scatter(fig[2, 1], 10 .^ (1:6) .* us"W/m^2", (1:6) .* 1000 .* u"nm"; axis=(; dim2_conversion=DQConversion(us"μm")))
scatter(fig[2, 2], (0:10) .* u"W/m^2", (0:10) .* u"g"; axis=(; dim1_conversion=DQConversion(us"W/m^2")))
scatter!(fig[2, 2], (0:10) .* u"kW/m^2", (0:10) .* us"kg")
fig
Happy to save that kind of discussion for a separate PR though if it's starting to get too in the weeds for this basic functionality PR. Thanks again for taking a look! |
|
This would be great for a project I'm working on right now. Can it be pushed? |
|
Hi @MilesCranmer, haha, I think I missed a notification too, sorry for the delay! Thanks for the comments, I think I have responded to all of them now for your review |
ext/DynamicQuantitiesMakieExt.jl
Outdated
| return Float64(conv) | ||
| end | ||
|
|
||
| # TODO: Maybe only allow symbolic units to avoid bugs? |
There was a problem hiding this comment.
Just completed. I've also updated the docs accordingly ef2d0d8
| using DynamicQuantities: UnionAbstractQuantity, ustrip, dimension | ||
| using TestItems: @testitem | ||
|
|
||
| import Makie as M |
There was a problem hiding this comment.
Just out of curiosity, is there a package higher up the plotting tools hierarchy that we should be overloading instead? For example there is RecipesBase.jl.
There was a problem hiding this comment.
You know, the distinction between MakieCore and Makie proper when it comes to defining recipes and axis conversions is something that I've been trying to clarify for myself too
AFAICT, MakieCore is for recipes, while axis converts (like in this PR) require the whole megillah so things like expand_dimensions and create_dim_conversion will work. At least, that is the sense I get from the docs, which seem to specifically advise using Makie over MakieCore for defining package extensions
Interestingly, it looks like the long term plan may be to phase out MakieCore, but idk what the latest on that is MakieOrg/Makie.jl#3645 (comment)
There was a problem hiding this comment.
Yep, looks like it's been absorbed into Makie now: https://makie.org/website/blogposts/v0.24.0/
|
One other question: @jkrumbiegel, do you think this functionality should live directly in Makie.jl, or is it better kept as an extension here? I’m a bit concerned about maintainability if we keep it here, since it relies on internal, non-public APIs of Makie.jl (though it uses the public API of DQ). Typically, in scenarios where an interface primarily relies on the internal details of package A (Makie.jl), but the public API of package B (DQ.jl), it feels safer to have it reside directly within package A as it lives closer to the people who might edit such internals. However, if there are strong objections, I’m fine with keeping it here. |
| @@ -504,3 +511,60 @@ function my_func(x::UnionAbstractQuantity{T,D}) where {T,D} | |||
| return x / ustrip(x) | |||
| end | |||
| ``` | |||
|
|
|||
| ### Plotting | |||
There was a problem hiding this comment.
Do we need an explicit example of this? i.e., is it something that users would expect to "just work" anyways?
Good question, Makie depends directly on Unitful so I'm not sure if we shouldn't add DynamicQuantities as an extension. But I didn't implement unit support so I'll ping @SimonDanisch here |
|
We're pretty tight on maintenance, so I'd prefer this to stay here. |
|
@SimonDanisch If @icweaver is happy to maintain the extension, could we move it into Makie? My main concern is that changes are far more likely on the Makie side than in DQ, so leaving it here risks silent breakage, whereas Makie’s CI would flag issues before release. It would remain an extension – no hard dependency – and we could add @icweaver to CODEOWNERS, meaning no extra maintenance for core Makie. @icweaver – would you be comfortable taking this on? |
|
I'm not sure what you mean by no extra maintenance if our CIs start failing and we can only move on once someone else fixed the failure ;) |
|
To be fair, if DQ is 100% stable, our CI should only fail if we actually change the interface. |
|
Hi folks, I just wanted to start by thanking Miles for making, imo, a nice alternative to Unitful, and to the Makie team for putting together such a slick system for pluggable dim conversions I'm happy to go wherever the code is and to help maintain things to the best of my ability. Thanks all again for your time =] |
|
@SimonDanisch Thanks for clarifying; those are fair concerns. Quickly addressing each:
The reason I'm suggesting Makie is mostly semantic: the extension depends primarily on Makie symbols rather than DQ's API. It feels more natural to keep it alongside those internals (similar to Unitful). That way, repo-wide edits (like renaming If that approach seems reasonable, we can quickly set up a minimal-overhead solution. But I'm happy to defer if you'd still prefer keeping it in DQ; I just wanted to clearly lay out the rationale first. |
|
If neither side currently wants to take on this glue code, one could also just make a package that implements the integration. It would not be as slick because you wouldn't get the benefits automatically with installing Makie and DynamicQuantities, but it's also much lower stakes. It could still be integrated into either DQ or Makie later. |
Benchmark Results (Julia v1.10)Time benchmarks
Memory benchmarks
|
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
|
Hi @MilesCranmer, here's the latest with Makie's new ComputePipeline now integrated in this PR for your review. Lots of cool stuff going on in their new release! |
|
@SimonDanisch @jkrumbiegel just to raise this again: I would really really prefer if Makie had this as an extension rather than DQ. As you can see from the latest commit on this PR, there are already edits going in from Makie’s latest version release. Whereas DQ is rock solid. I don’t want the releases of DQ to simply be re-releasing a patch whenever Makie makes a change. Once this part of the API is guaranteed to be stable, I am happy to have it here, but until then, I think it doesn’t make sense to have me maintaining a list of symbols for an unrelated package. Makie already stores Unitful’s extension and they are quite a very similar if not identical API (in fact there have been discussions of having a lightweight AbstractUnits package to formalise this) so I don’t think it will be that bad to include DQ there too. We all have a huge maintenance load; I would say that this is about reducing the combined load on us, which I believe it would. And thanks @icweaver for your efforts, its much appreciated. Would it be alright if you make this as a PR to Makie? Hopefully the above reasoning makes sense to you - I really appreciate the effort, I just think the location isn’t optimal. |
|
Hi Miles, totally understand. Happy to get a PR + docs going on Makie's side if their team is open to it. I think this would also help make using our DynamicQuantities.jl/docs/src/examples.md Lines 533 to 534 in 2f7086e |
|
I guess we can do that;) |
Closes #164
Demo
See docs for more examples using Makie's dimension conversion machinery
TODO
Future PR(s)?