Skip to content
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

Request: Leverage package extensions #331

Closed
jakobnissen opened this issue Jan 30, 2023 · 7 comments · Fixed by #332
Closed

Request: Leverage package extensions #331

jakobnissen opened this issue Jan 30, 2023 · 7 comments · Fixed by #332

Comments

@jakobnissen
Copy link

In the upcoming Julia 1.9, dependencies can be added as optional, such that they are only loaded if a set of other packages are also loaded in the same session.

UnicodePlots is quite dependency-heavy (see #291), leading to high latency (see #324) but the deps can't just be removed as this would remove features in some cases.
Using package extensions could significantly improve loading times and latency for the large majority of users, while still providing the features needed by the minority.

Package extensions are non-breaking on earlier Julia versions.

@t-bltg
Copy link
Member

t-bltg commented Feb 4, 2023

I'd like to correct a few things here:

I find it rather cumbersome to request from a user to load ColorSchemes to use basic plot features (this isn't what I'd call minor usage) such as colormaps (used by contourplot, surfaceplot and heatmap) before using UnicodePlots.

That being said, we can support package extensions for the single @require line present in __init__:

Requires.@require ImageInTerminal = "d8c32880-2388-543b-8c61-d9f865259254" begin
, but I must admit this isn't on my todo list (there currently is no added value to doing this except making the code more complex, and of course modernize it as julia enhances). We could maybe wait for the next LTS and support only julia versions with package extensions thus dropping the Requires usage at this precise moment.

We can maybe make the FreeType dependency weak, that would make sense (.png export is not much used).

EDIT: I finally went ahead and added FreeType, FileIO and ImageInTerminal as package extensions in #332.

@t-bltg
Copy link
Member

t-bltg commented Feb 4, 2023

See JuliaGraphics/ColorVectorSpace.jl#186 for the proposal to make SpecialFunctions optional in ColorVectorSpace.

@mcabbott
Copy link

mcabbott commented Feb 5, 2023

From #324 StaticArrays and Unitful seem to take a long time. Are SArrays used internally? Quick searches did not tell me much. Edit, yes, e.g. SMatrix rotations, not easy to avoid.

But Unitful seems to be fairly isolated, might also be a good candidate?

function lineplot(
x::AbstractVector{<:RealOrRealQuantity},
y::AbstractVector{<:Quantity};

@t-bltg
Copy link
Member

t-bltg commented Feb 5, 2023

Are SArrays used internally? Quick searches did not tell me much.

I knew this felt like déjà vu: #222 (comment) ;)

But Unitful seems to be fairly isolated, might also be a good candidate?

It's actually used in two other places:

number_unit(x::AbstractVector{<:Quantity}, fancy = true) =
ustrip.(x), unit_str(first(x), fancy)
number_unit(x::Quantity, fancy = true) = ustrip(x), unit_str(x, fancy)

and
# Unitful
function scatterplot(
x::AbstractVector{<:RealOrRealQuantity},
y::AbstractVector{<:Quantity};
unicode_exponent::Bool = KEYWORDS.unicode_exponent,
xlabel = KEYWORDS.xlabel,
ylabel = KEYWORDS.ylabel,
kw...,
)
x, ux = number_unit(x, unicode_exponent)
y, uy = number_unit(y, unicode_exponent)
scatterplot(
ustrip.(x),
ustrip.(y);
xlabel = unit_label(xlabel, ux),
ylabel = unit_label(ylabel, uy),
unicode_exponent,
kw...,
)
end
scatterplot!(
plot::Plot{<:Canvas},
x::AbstractVector{<:RealOrRealQuantity},
y::AbstractVector{<:Quantity};
kw...,
) = scatterplot!(plot, ustrip.(x), ustrip.(y); kw...)

As you said this is a good candidate too. You likely have Unitful loaded when you plot Quantities.
One drawback of using extensions is that it fragments the code so I have to think a bit more about the implementation in #332.

EDIT: implemented UnitfulExt as well.

@mcabbott
Copy link

mcabbott commented Feb 5, 2023

déjà vu

Sorry! I searched but clearly not everywhere...

#332 looks great.

@Moelf
Copy link
Member

Moelf commented Feb 22, 2023

Edit, yes, e.g. SMatrix rotations, not easy to avoid.

it is used, but do we need to use it? like, are people plotting into text-based terminal in a loop?

for the 1+ seconds of load time caused by StaticArrays.jl, we can probably create and rotate 100 times and not all plots need rotational matrix I'm guessing

@t-bltg
Copy link
Member

t-bltg commented Feb 22, 2023

While implementing 3d plots, I didn't take loading time into account and thought using static matrices was a good idea (using the assumption that people use a lot StaticArrays so it would already be loaded). So I didn't benchmark using regular matrices vs StaticArrays ones. I can imagine the case where you plot the 3d results from a pde evolving in time thus using a fixed model-view-projection matrix. I can also imagine rotating stuff (azimuth and elevation) and generating a gif out of it (changing MVP matrix), so there are multiple use cases here.

I'm all in favour of removing the StaticArrays dependency for 3d plots if the performance impact is negligible (a few percents would be acceptable).

I'm marking this on my Todo list (we need a plausible real case usage benchmark).

An alternative would be to adapt the code using weak deps as well for StaticArrays (and have a fallback version using regular matrices), but I'm not sure this is feasible.

I've opened #335 to track the StaticArrays issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants