-
Notifications
You must be signed in to change notification settings - Fork 25
[OLD] Plotting with Makie.jl #165
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
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")
fig or 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 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 |
I guess we can do that;) |
haha, cool, just opened MakieOrg/Makie.jl#5137 |
Ok, just updated things here so that it is a companion docs PR to the functionality PR in Makie now |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
=======================================
Coverage ? 99.21%
=======================================
Files ? 21
Lines ? 1273
Branches ? 0
=======================================
Hits ? 1263
Misses ? 10
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks; could you make a new PR for that though? This thread is kinda long and unrelated |
haha, for sure #183 |
Closes #164
Demo
See docs for more examples using Makie's dimension conversion machinery
TODO
Future PR(s)?