Add unit axis attribute, for Plots v1#5098
Add unit axis attribute, for Plots v1#5098BeastyBlacksmith merged 23 commits intoJuliaPlots:masterfrom
unit axis attribute, for Plots v1#5098Conversation
|
CI is failing on this one. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5098 +/- ##
==========================================
+ Coverage 89.77% 89.83% +0.06%
==========================================
Files 40 40
Lines 8780 8872 +92
==========================================
+ Hits 7882 7970 +88
- Misses 898 902 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR isn't quite ready yet, because I think it deserves a few more tests for the UnitfulExt before merging, plus some additions to the docs. But @t-bltg the CI failures seem to be either 1) a resurface of a PlotlyJS>Blink>Electron issue #5075, which is randomly hitting only some of the Ubuntu test cases or 2) something to do with the 1.12 prerelease |
Ignore those (they are development aids), we focus on julia |
|
Progress:
|
|
@gustaphe, I realized today that I was assuming the Edit: |
|
@Ickaser can you post mwe code and resulting images in your PR description, so we can see in a glance what changes ? |
Sure, I've added that to the PR description and linked to the relevant issues. |
Not an unfair assumption, that would probably have been a better design choice. |
This comment was marked as outdated.
This comment was marked as outdated.
|
We do need all of these for 1.x releases or don't we? |
You are right. |
I think I did it right then--for 1.x, the docs changes are in JuliaPlots/PlotDocs.jl#360, and for 2.0(#5095), the docs changes are in the single PR to the monorepo. |
Co-authored-by: t-bltg <tf.bltg@gmail.com>
|
I think this is generally good, but I'm hunting down a regression. The second argument to plot(rand(5)*u"m"; yguide="x", unitformat=latexify)should have the yguide |
src/axes.jl
Outdated
| ustr = if Plots.backend_name() ≡ :pgfplotsx | ||
| Latexify.latexify(axis[:unit]) | ||
| else | ||
| string(axis[:unit]) | ||
| end | ||
| isempty(axis[:guide]) && return ustr | ||
| return format_unit_label( | ||
| axis[:guide], | ||
| ustr, | ||
| axis[:unitformat]) |
There was a problem hiding this comment.
| ustr = if Plots.backend_name() ≡ :pgfplotsx | |
| Latexify.latexify(axis[:unit]) | |
| else | |
| string(axis[:unit]) | |
| end | |
| isempty(axis[:guide]) && return ustr | |
| return format_unit_label( | |
| axis[:guide], | |
| ustr, | |
| axis[:unitformat]) | |
| isempty(axis[:guide]) && return string(axis[:unit]) | |
| return format_unit_label( | |
| axis[:guide], | |
| axis[:unit], | |
| axis[:unitformat]) |
There was a problem hiding this comment.
This together with somehow setting the default unitformat for pgfplotsx to (l, u)->"\$$l\\;\\left($(Latexify.latexify(u; env=:raw))\\right)\$" solves my problem. I had success putting default(unitformat=(l, u)->"\$$l\\;\\left($(Latexify.latexify(u; env=:raw))\\right)\$") at line 1608 of src/backends.jl (inside _initialize_backend(pkg::PGFPLotsXBackend)), but I don't know if that's legal.
There was a problem hiding this comment.
I would like to find a solution that doesn't involve setting a default in the backend, if I can?... So far the best idea I am coming up with is that PGFPlotsX should have its own function (or set of methods) for format_unit_label(l, u), that puts \\left and \\right on all the characters.
That, or for all the cases except unitformat <: Function, latexify gets called on the unit before getting passed to format_unit_label. That way if unitformat=latexify gets passed, latexify gets to see the unit object itself, but if any of the defaults are used then things still work.
Increasingly I find myself of the opinion that in v2 it might be worth dropping most of the methods of format_unit_label--unless I'm using :round or :square, I almost always end up writing a function myself, and I think it's not too complicated an operation for most users. (Probably not more complicated than understanding how many symbols they want to pass for the current methods, at least.)
There was a problem hiding this comment.
I went for the unitformat isa Function check and am letting tests run now: if this works in your cases, I might like this solution best since it doesn't involve setting any defaults, but if the resulting code is unclear I am open to other suggestions (like your original one).
Sure enough, in |
|
The latest CI failures are either 1) failure to precompile Gtk.jl on Windows (seems random: affected 1.10 and 1.11, not 1.6, and worked the last time CI ran) and 2) downstream GraphRecipes failing on a Julia type tree (which fails on 1.11 and 1.12 across branches, it seems). I added one (1) test with latexify to the test suite, and it seems to work now, so if that's enough (and latexify solution looks good to @gustaphe) I think this is good to go. |
|
Otherwise LGTM. Good work! |
* Catch some fixes from #5098 and #5127 for v2 * Copypaste whitespace formatting * EOF whitespace format * Load latexify for Unitful tests * Add improvements from #5158 * PlotsBase, not Plots * Fully excise UnitfulLatexify, and drop Latexify as a dep for Unitful * no need for brackets * Fix unitful latexify tests * Make Latexify and LaTeXStrings full deps of PlotsBase, remove hack * Add Colors and Contour as full deps of PlotsBase, so PGFPlotsXExt triggers with no extra imports * Allow Colors, Contour, LaTeXStrings, Latexify to be stale deps for PlotsBase * Add a test to improve coverage * Another test suggested by coverage * do that test right, whoops * Drop "WEAKDEPS" for triggering extensions
Description
Essentially identical to #5095 , but made against the
masterbranch so that I'm not waiting for Plots v2--folder structures are different enough that it made sense to open as a separate PR.@gustaphe and I discussed some in #5095; it would be helpful to get feedback from @t-bltg or @BeastyBlacksmith or someone else who is not as focused on Unitful usage.
A lot of the noise in the diff is a result of switching to
Plots.get_guide(axis)instead ofaxis[:guide], because that new function allows us to more elegantly handle differing unit formats in the guide strings; in any case where the axis has no units, the result ofget_guide(axis)isaxis[:guide]so not a big change in behavior.Other changes involve moving some of the machinery out of the UnitfulExt so that it is accessible within Plots for axis handling.
MWEs showing new behavior
(So: this PR is fixing #4822.)
(So, this PR is fixing #4741 and #4750, although the MWEs in both of those issues do not plot correctly into the twinned axis.)
Fix #4947.
Fix #4822.
Attribution
Things to consider