-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
RFC: Refactor Plots #1598
RFC: Refactor Plots #1598
Conversation
Also: Julia 0.6 julia> @time using Plots
23.636787 seconds (6.36 M allocations: 359.742 MiB, 0.96% gc time) Julia 0.7 Plots#master julia> @time using Plots
5.960243 seconds (11.26 M allocations: 621.526 MiB, 7.59% gc time) Julia 0.7 Plots#refactor julia> @time using Plots
5.059459 seconds (10.87 M allocations: 588.263 MiB, 7.35% gc time)
julia> @time using PlotsPyPlot
10.296359 seconds (25.74 M allocations: 1.257 GiB, 5.81% gc time) @time using PlotsPyPlot
14.948731 seconds (35.50 M allocations: 1.779 GiB, 6.45% gc time) |
I've been pretty busy lately but here's what I understand from this and what I think. So in order to have PyPlot running with Plots on 0.7 we'd have to list it as a dependency. If that is the case I'm definitely in favour of the refactor. What is weird is that Plotly works even though we don't list it as a dependency? Also it seems like it shaves off a decent bit from the load time, which is a nice bonus 😄 . Overall I'm in favour of these changes! Great Work @daschw !! As for GR I do think it'd make sense to drop the dependency and update PlotsGR for consistency purposes. |
Thanks @apalugniok ! Plotly is no julia package, just a download of a js file. Apparently (#918 (comment)) there is another solution using Requires now, which I am currently investigating. So I will report back when I have a comparison. |
Great work! Most of my Julia time is spent on my GSoC now so it's hard for me to contribute. I just wanted to say that I think Requires is a good solution if it works reliably. One thing to check to make sure that it works is the using Plots
plot(rand(10)) on Juno causes this error and it's a good test for the reorganization to see if this gets fixed. FWIU the advantage of Requires compared to how it is now is that backend specific code is not loaded unless necessary so this should speed things up at start up. If instead you see that Requires solution doesn't work satisfactorily I can maybe share the backend organization of Interact, as I've been quite happy with it so far, but switching to that model would require a fair amount of work. |
The above script works for me. To induce the world-age-problem, I had to: using Plots
display(plot(rand(10)))
|
The problem with the Requires approach and PyPlot (or any other package): function __init__()
@require PyPlot=[uuid] include(joinpath(@__DIR__, "backends/pyplot.jl"))
...
end if I have no I hope that I am missing the obvious, easy and correct implementation 😟 . |
AFAIU, It should be the user to do that, meaning the correct usage would be: using Plots
import PyPlot
plot(...) |
OK, thanks @piever! I will try this later |
That's why the user should do: using Plots
import PyPlot
plot(...) If PyPlot is imported (without I wonder whehter:
would also be sufficient to trigger the code in the |
Interesting idea, I will check this. |
I experimented a little with two example packages PkgA and PkgB: PkgACorresponds to Plots with a default backend and conditional dependency on a second backend PkgB via Requires. Both export module PkgA
abstract type Backend end
export BackendA, BackendB, func, a, b
struct BackendA <: Backend end
struct BackendB <: Backend end
func(x, ::Type{BackendA}) = string(x)
mutable struct CurrentBackend
pkg
end
CB = CurrentBackend(BackendA)
func(x) = func(x, CB.pkg)
a() = CB.pkg = BackendA
function b()
if !(:PkgB in Symbol.(collect(values(Base.loaded_modules))))
@eval import PkgB
end
CB.pkg = BackendB
end
using Requires
function __init__()
@require PkgB = "7a818804-8b6b-11e8-33ae-c961580b5757" begin
func(x, ::Type{BackendB}) = PkgB.func(x)
end
end
end # module PkgBmodule PkgB
export func
func(x) = Symbol(x)
end # module Unfortunately @piever's julia> using PkgA
julia> b()
ERROR: ArgumentError: Package PkgA does not have PkgB in its dependencies:
- If you have PkgA checked out for development and have
added PkgB as a dependency but haven't updated your primary
environment's manifest file, try `Pkg.resolve()`.
- Otherwise you may need to report an issue with PkgA.
Stacktrace:
[1] require(::Module, ::Symbol) at ./loading.jl:821
[2] eval at ./boot.jl:319 [inlined]
[3] b() at /home/dani/.julia/dev/PkgA/src/PkgA.jl:21
[4] top-level scope at none:0 However, it seems to wotk without the function b()
if !(:PkgB in Symbol.(collect(values(Base.loaded_modules))))
@warn("run `import PkgB` before running `b()`.")
else
CB.pkg = BackendB
end
end Usagejulia> using PkgA
julia> func(0)
"0"
julia> b()
┌ Warning: run `import PkgB` before running `b()`.
└ @ PkgA ~/.julia/dev/PkgA/src/PkgA.jl:21
julia> import PkgB
julia> b()
BackendB
julia> func(0)
Symbol("0")
julia> a()
BackendA
julia> func(0)
"0" So I guess our decision between reorg and Requires boils down to whether we want the user to do |
As an aside, I suspect that |
Nice! julia> using PkgA
julia> b()
BackendB
julia> func(0)
Symbol("0") However, if the package |
So we could also keep the current structure without Requires and replace the |
I would tend to prefer the Requires solution because it avoids unnecessary code loading. As it is now, even though one may only use GR, Plots still loads all backend codes. We should probably measure it, but if Requires, by removing the need to load some of these files, speeds up start up time, we should probably go for it. To be honest though I find the: using Plots; import PyPlot approach saner and cleaner. One thing we could add is to put the using Plots; import PyPlot the backend is set (though maybe we should put some rules, like saying that this sets the backend only the first time one of the backends is loaded). |
See #1601 for an alternative solution using Requires including benchmarks. |
GR and Plotly currently work on 0.7 on master. However, when I try to use PyPlot, I get the following:
It seems like we would have to have PyPlot in our dependencies for this to work. The same applies for all the other backends.
Hence, I propose to finally do the reorg of Plots with backend glue packages to get rid of the optional dependencies.
With this PR Plots only natively supports GR and Plotly. For all the other backends (still to be created) glue packages have to be installed and loaded. I have already implemented a PlotsPyPlot backend package locally that I plan to upload soon.
With this branch and the backend package the workflow is like this:
You can now still switch backends with
gr()
,plotly()
andpyplot()
.using PlotsPyPlot
automatically callspyplot
.I plan to implement further backends, whenever I find time and they are ready for 0.7, but I would really appreciate some help, maybe also with updating the docs.
I also wonder, if we should drop the GR dependency again with this refactor and update the PlotsGR package. It seems kind of weird that users need to install GR if they'd only like to use PlotsPyPlot or PlotsUnicodePlots for example.
I would really appreciate your feedback on this.
cc: @mkborregaard @piever @apalugniok @jheinen @ChrisRackauckas @Evizero