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

Adding arbitrary legend_columns for python-backends #4678

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

briederer
Copy link
Contributor

@briederer briederer commented Mar 3, 2023

As a follow up to #4645 I am trying to implement the feature also for other backends.

As asked in #4645 (comment) by @lmanzanillas I have started with the PythonPlot backend, which has been rather trivial in principle. However, I could not reproduce the behaviour of a fully horizontal legend for legend_columns=-1 like in gr(). I.e. the legend_title being on the same line as the series labels.

I'll have a look at the other backends too and then also create Test Images for the docs.

Samples

data = rand(10, 4)
leg_cols = -2:5

leg_plots(; kw...) = begin
  map((leg_col -> begin
      plot(data; marker=:circle, ticks=:none, plot_title=string("legend_columns = ", leg_col), legend_columns=leg_col, kw...)
    end), leg_cols)
end
(w, h) = (600,600)
with(scalefonts=0.5, size=(w, h)) do
  plot(leg_plots()..., leg_plots(legend_title="Test")...; layout=(4, 4))
end

GR

sample_gr

┌ Warning: n° of legend_column=0 has undefined behaviour. Assuming vertical layout.
└ @ Plots ~/.julia/dev/Plots/src/backends/gr.jl:1226
┌ Warning: n° of legend_column=4 is larger than n° of series=3
└ @ Plots ~/.julia/dev/Plots/src/backends/gr.jl:1223
┌ Warning: n° of legend_column=0 has undefined behaviour. Assuming vertical layout.
└ @ Plots ~/.julia/dev/Plots/src/backends/gr.jl:1226
┌ Warning: n° of legend_column=4 is larger than n° of series=3
└ @ Plots ~/.julia/dev/Plots/src/backends/gr.jl:1223

PythonPlot

sample_pythonplot

┌ Warning: n° of legend_column=0 has undefined behaviour. Assuming vertical layout.
└ @ Plots ~/.julia/dev/Plots/src/backends/pythonplot.jl:1438
┌ Warning: n° of legend_column=4 is larger than n° of series=3
└ @ Plots ~/.julia/dev/Plots/src/backends/pythonplot.jl:1435
┌ Warning: n° of legend_column=0 has undefined behaviour. Assuming vertical layout.
└ @ Plots ~/.julia/dev/Plots/src/backends/pythonplot.jl:1438
┌ Warning: n° of legend_column=4 is larger than n° of series=3
└ @ Plots ~/.julia/dev/Plots/src/backends/pythonplot.jl:1435

Other Backends

To be done.

@briederer
Copy link
Contributor Author

Also interesting to note:
PythonPlot orders the series differently in the multi-column layout (i.e. column-major). Since this is also the way how julia treats matrices it is probably the preferred way. So I would suggest tho change GR to this behaviour too.

@briederer
Copy link
Contributor Author

briederer commented Mar 3, 2023

Apparently for the some most backends the legend_column=-1 possibility is not even implemented so I won't touch it.
The only remaining backend is therefore Gaston. However, I am currently on a Mac without Gnuplot so I can't test that.

So although my plans where ambitious, I think adding this option for python backends is all I can do at the moment.

@briederer briederer changed the title Adding arbitrary legend_columns Adding arbitrary legend_columns for python-backends Mar 3, 2023
@briederer briederer marked this pull request as ready for review March 3, 2023 13:52
@BeastyBlacksmith
Copy link
Member

Also interesting to note: PythonPlot orders the series differently in the multi-column layout (i.e. column-major). Since this is also the way how julia treats matrices it is probably the preferred way. So I would suggest tho change GR to this behaviour too.

FWIW pgfplots also sorts these row-major

@briederer
Copy link
Contributor Author

Hmm 🤔
I mean zo be fair probably is row-major a bit more intuitive in a horizontal legend since (at least in the english language) we are reading fromm left to right and then jump in the next line.
I don't have any strong opinions, except for wanting a common behavior for the different backends.

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Patch coverage: 62.50% and project coverage change: -0.02 ⚠️

Comparison is base (4916cf8) 90.42% compared to head (a159c43) 90.40%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4678      +/-   ##
==========================================
- Coverage   90.42%   90.40%   -0.02%     
==========================================
  Files          40       40              
  Lines        8703     8706       +3     
==========================================
+ Hits         7870     7871       +1     
- Misses        833      835       +2     
Impacted Files Coverage Δ
src/backends.jl 82.58% <ø> (ø)
src/backends/gr.jl 90.65% <50.00%> (ø)
src/backends/pythonplot.jl 87.44% <66.66%> (-0.24%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BeastyBlacksmith
Copy link
Member

It would be possible to let the sign carry the majorness.
Not sure, if I'd like that though ^^

@briederer
Copy link
Contributor Author

briederer commented Mar 3, 2023

I also thought about that, but then -1 for horizontal would probably not be the right choice anymore but instead -nseries. So I think that this is not the best solution.
And in any case people can always sort their data if they want to swap the legend-entries.

@t-bltg
Copy link
Member

t-bltg commented Mar 9, 2023

See also #2047.
I would be in favor of unifying to row major now for consistency with current layout behavior, and switch to col major in Plots 2.0 (breaking), even if I hear the natural left to right reading argument.

@briederer
Copy link
Contributor Author

That's a good point.
I'll change it tomorrow. Since it's already there but only commented it should not take too much time.

However, I also want to give the pgfplotsx backend another look.
Since it is implemented apparently in the backend it should be doable.

@BeastyBlacksmith
Copy link
Member

However, I also want to give the pgfplotsx backend another look.
Since it is implemented apparently in the backend it should be doable.

It is already working for that. Consider:

julia> using Plots; pgfplotsx()
Plots.PGFPlotsXBackend()

julia> plot(rand(4,4), legend_columns = 2)

Its just that your example errors because pgfplots errors on too small labels for some reason...

@briederer
Copy link
Contributor Author

That's good to know.
Thanks 😊
Then it just needs to be added to the supported attributes.

@BeastyBlacksmith
Copy link
Member

@briederer is this good to go?

@briederer
Copy link
Contributor Author

Oh sorry, totally forgot about this.
I am not sure if GR and python are consistent at the moment and it still lacks proper docs and/or TestImages.
I am currently very limited in my time, but I'll have another look next week (unless someone else has time and wants to jump in 😅).

@BeastyBlacksmith
Copy link
Member

bump

@BeastyBlacksmith
Copy link
Member

kindly reminder

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

Successfully merging this pull request may close these issues.

3 participants