Closed gustaphe closed 3 years ago
Currently, calling plot!(f, u)
on an already unitful figure will do the right thing. Calling plot(f, u)
sets no unit for the x
axis. To do so would require us to either replicate the behavior of plotting unitless functions (including finding a suitable x
vector), or to pretty much run the entire vector-vector recipe. I'm not in love with either approach, but I don't think we can really merge like this.
The first approach is probably going to be better, but I've looked into the way Plots plots functions before, and there's a bit of voodoo going on in there. I'll see what I can do.
Merging #50 (7164808) into master (d0508b7) will decrease coverage by
0.45%
. The diff coverage is75.00%
.
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
- Coverage 85.71% 85.26% -0.46%
==========================================
Files 1 1
Lines 91 95 +4
==========================================
+ Hits 78 81 +3
- Misses 13 14 +1
Impacted Files | Coverage Δ | |
---|---|---|
src/UnitfulRecipes.jl | 85.26% <75.00%> (-0.46%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 42a9a37...7164808. Read the comment docs.
Currently, calling
plot!(f, u)
on an already unitful figure will do the right thing. Callingplot(f, u)
sets no unit for thex
axis. To do so would require us to either replicate the behavior of plotting unitless functions (including finding a suitablex
vector), or to pretty much run the entire vector-vector recipe. I'm not in love with either approach, but I don't think we can really merge like this.The first approach is probably going to be better, but I've looked into the way Plots plots functions before, and there's a bit of voodoo going on in there. I'll see what I can do.
Instead of replicating the approach, maybe there can be some upstream refactoring be done? For instance, expose the machinery that guesses a reasonable domain for f
?
My guess is, if these lines
were in a function _functionlimits(f, plotattributes)
, then we could do
@recipe function f(f::Function, u::Units)
(xmin, xmax) = RecipesPipeline._functionlimits(f, plotattributes)
f, xmin*u, xmax*u
end
and that would do everything we want.
I guess that's kind of what _apply_recipe
does. How's this for an idea:
That test failure is bizarre to me. On macOS-latest, the "m^2"
label is turned to "m²"
. It runs on a slightly older version of Julia than ubuntu-latest, but I checked the diff between those two commits and there is nothing that seems even related. Maybe there's a difference in a package, I think I'll just ask it to rerun in a couple of days.
On macOS-latest, the
"m^2"
label is turned to"m²"
I don't have lots of time (and skill 😅) to properly investigate, but FWIW, I recall that in https://github.com/PainterQubits/Unitful.jl/pull/297 the Unitful.jl dev set the default to use Unicode superscripts for OSX and Linux and use "^"
for Windows, so maybe that's what fails there?
Yeah, that was probably it. This test circumvents that.
Only the @recipe
lines uncovered now, so RFC.
Very little time to comment on the changes but LGTM. And you can safely ignore the missed lines because I think that's a Julia/Coverage.jl issue! 🙂
Sorry about the noise, I thought of another case. Functions where the domain must be unitful (like exp(x/3u"m")
) didn't work before. It's sorted now, but it requires this more complicated approach.
I looked into the possibility of making function plots (
plot(f)
) work automatically, but since we don't (and can't) set the limits to unitful quantities, I think that's not going to happen.To me, the second best thing is being able to plot a function against all of the
x
axis, while supplying the unit of thex
axis. Heuristically, "plot this function against 'seconds'". This is easier than figuring out how to modify a function so that it works in a unitful plot (actually justplot(x->f(x*u))
, but that always takes me a while to wrap my head around).Also documents the existing
plot(f, x)
.Theoretically, we could use any unoccupied signature and use the axis unit parameter, but I couldn't really think of a simpler signature than this. Maybe it should ignore the value of the unit argument, as it stands
f(model, u"ms")
doesn't make any sense (thex
axis will be off by a factor 1e3), andf(model, u"kg")
will straight up fail, so you're not really communicating anything more than that you're expecting a unitful axis anyway.