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

implements best legend place holder #4536

Merged
merged 34 commits into from
Nov 24, 2022

Conversation

lmiq
Copy link
Contributor

@lmiq lmiq commented Nov 21, 2022

Implemented the best legend position placeholder: It will choose between bottomleft/right or topleft/right, depending on a sample of the data, trying to maximize the difference to the closest point. The function is fast (5-10 microseconds), and should be good enough for a guess of the best position.

Tries to address: #3399

Test it with:

using Plots
scatter(randn(100), randn(100))

@t-bltg
Copy link
Member

t-bltg commented Nov 21, 2022

Nice, I'm going to try it now.

Don't forget to run julia -e 'using JuliaFormatter; format(["src", "test"])' and commit for the format check to pass.

@t-bltg t-bltg added the extension new behaviour label Nov 21, 2022
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
lmiq and others added 2 commits November 21, 2022 12:36
Co-authored-by: t-bltg <tf.bltg@gmail.com>
Co-authored-by: t-bltg <tf.bltg@gmail.com>
@t-bltg
Copy link
Member

t-bltg commented Nov 21, 2022

Here are the changed reference images (25 out of 65).

Looking weird: refs 7, 15, 19.

ref4
ref4
ref7
ref7
ref8
ref8
ref12
ref12
ref13
ref13
ref15
ref15
ref17
ref17
ref18
ref18
ref19
ref19
ref23
ref23
ref25
ref25
ref26
ref26
ref29
ref29
ref30
ref30
ref36
ref36
ref37
ref37
ref44
ref44
ref46
ref46
ref48
ref48
ref51
ref51
ref53
ref53
ref56
ref56
ref58
ref58
ref61
ref61
ref62
ref62

src/backends/gr.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
Co-authored-by: t-bltg <tf.bltg@gmail.com>
@lmiq
Copy link
Contributor Author

lmiq commented Nov 21, 2022

Here are the changed reference images (25 out of 65).

Actually most of them look weird to me, it is not what I was expecting. But now that I know those tests exist, I can check each of them before committing changes.

(The histogram is case is a case apart, as the plot does not show the data series, and what I do know is based on the data series).

@t-bltg
Copy link
Member

t-bltg commented Nov 21, 2022

But now that I know those tests exist, I can check each of them before committing changes.

The refs files are checked when running test Plots.

You can set julia> ENV["VISUAL_REGRESSION_TESTS_AUTO"] = true before pkg> test Plots (see

if get(ENV, "VISUAL_REGRESSION_TESTS_AUTO", "false") == "true" && name != "backends"
) in order to update them automatically (no prompt), and partially run the test-suite (faster).

See also https://docs.juliaplots.org/stable/contributing/#VisualRegressionTests.

Co-authored-by: t-bltg <tf.bltg@gmail.com>
@t-bltg
Copy link
Member

t-bltg commented Nov 21, 2022

To quickly run an example, you can use Plots.test_examples(3).

@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Base: 90.89% // Head: 90.95% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (569912d) compared to base (816d0f4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4536      +/-   ##
==========================================
+ Coverage   90.89%   90.95%   +0.06%     
==========================================
  Files          40       40              
  Lines        7707     7740      +33     
==========================================
+ Hits         7005     7040      +35     
+ Misses        702      700       -2     
Impacted Files Coverage Δ
src/examples.jl 97.26% <ø> (ø)
src/backends/gr.jl 91.66% <100.00%> (-0.01%) ⬇️
src/utils.jl 92.93% <100.00%> (+0.41%) ⬆️
RecipesPipeline/src/type_recipe.jl 94.44% <0.00%> (+5.55%) ⬆️

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.

@t-bltg
Copy link
Member

t-bltg commented Nov 21, 2022

Actually most of them look weird to me, it is not what I was expecting.

Maybe we need to add some weights in the algorithm to favor the :topright position.

@lmiq
Copy link
Contributor Author

lmiq commented Nov 21, 2022

I'll close the PR for now to work on it further without disturbing you.

@lmiq lmiq closed this Nov 21, 2022
@t-bltg
Copy link
Member

t-bltg commented Nov 21, 2022

You could also have marked it as a draft (it disables notifications ;)).

@lmiq lmiq reopened this Nov 22, 2022
@lmiq
Copy link
Contributor Author

lmiq commented Nov 22, 2022

Corresponding image PR: JuliaPlots/PlotReferenceImages.jl#142

@lmiq lmiq marked this pull request as ready for review November 22, 2022 18:37
Copy link
Member

@t-bltg t-bltg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like @BeastyBlacksmith to comment before merging the reference images.

To me, there is no ambiguity on the automatic placement (at least on the ref images).

@lmiq
Copy link
Contributor Author

lmiq commented Nov 22, 2022

I just updated a comment. Nothing will change.

@lmiq
Copy link
Contributor Author

lmiq commented Nov 22, 2022

To me, there is no ambiguity on the automatic placement (at least on the ref images).

I don't think there is any ambiguity. The position is deterministic, given the data set (which is a difference from the original implementation I posted on the discourse forum, which used a random sample from the data set).

Copy link
Member

@BeastyBlacksmith BeastyBlacksmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be more happy if it kept :topright in the cases where it is impossible not to overlap with data (or maybe even choose :outertopright then).
But I like it either way. Good work!

src/utils.jl Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
@lmiq
Copy link
Contributor Author

lmiq commented Nov 24, 2022

where it is impossible not to overlap

If we could obtain the coordinates of the legend box we could try that. But I didn't find how to do it, so we are just examining the plot edges relationship with the data. We can try some different heuristics, such as summing some distances, but we would need a case where the current strategy is clearly bad. Also biased examples against any other strategy should be tested.

src/utils.jl Outdated Show resolved Hide resolved
@lmiq
Copy link
Contributor Author

lmiq commented Nov 24, 2022

I'll post the results of this different placement strategy, which does better if there is a bunch of points in one corner, but the closest overlap is in the other (should I have created a new branch? I'm still confused with this)

@lmiq
Copy link
Contributor Author

lmiq commented Nov 24, 2022

Here they are: https://github.com/JuliaPlots/PlotReferenceImages.jl/pull/143/files

It actually looks good, IMO. Maybe better, particularly in the scatter plots.

(I'll be away for a couple of hours now)

@t-bltg
Copy link
Member

t-bltg commented Nov 24, 2022

Thanks, merged, let's try that now.
There are indeed less changes w.r.t. the original ref imgs, making this PR less controversial.

@t-bltg
Copy link
Member

t-bltg commented Nov 24, 2022

I've have cleaned up reference images between versions 1.36.3 and 1.36.4 ("only" 21 images changed by this PR).

@t-bltg
Copy link
Member

t-bltg commented Nov 24, 2022

Should we merge ?
EDIT: merging now, since it blocks other PRs, because of the reference images.

@t-bltg t-bltg merged commit 3940347 into JuliaPlots:master Nov 24, 2022
@t-bltg
Copy link
Member

t-bltg commented Nov 24, 2022

Thanks @lmiq, this is a really nice contribution 🎉 .

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

Successfully merging this pull request may close these issues.

3 participants