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

**BREAKING** change of sampleplot behaviour and defaults #213

Merged
merged 27 commits into from
Aug 26, 2021

Conversation

st--
Copy link
Member

@st-- st-- commented Aug 24, 2021

This PR makes two breaking changes to the sampleplot plotting utility:

  1. Most default settings have been removed.
    • sampleplot no longer imposes a default seriescolor.
      How to obtain the previous color: manually pass seriescolor = "red".
    • The markers have been removed as they seemed more confusing than helpful (overwhelmingly large in the legend, almost invisibly tiny on the plot). The linealpha default has been increased to 0.35 to keep visual appearance the same.
      How to obtain the previous markers: If you actually liked the tiny markers, check the diff for the previous default settings that you now need to pass manually.
  2. Multiple samples (samples kwarg > 1) are now plotted as a single series. This means, for example, that passing a string as label only adds one legend entry for all samples, not one per sample.
    How to obtain previous behaviour: If you actually want each sample to be a different series (for example, to give them different colors), you now need to call sampleplot multiple times instead.

Additionally, this PR contains the following non-breaking minor changes:

  • minor fix: error message for check of ribbon_scale in plot recipe
  • internal code cleanup (pop! for custom-defined kwarg)

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2021

Codecov Report

Merging #213 (36f91b3) into master (4423ebc) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 36f91b3 differs from pull request most recent head ab37628. Consider uploading reports for the commit ab37628 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
- Coverage   98.05%   98.02%   -0.03%     
==========================================
  Files          10       10              
  Lines         359      355       -4     
==========================================
- Hits          352      348       -4     
  Misses          7        7              
Impacted Files Coverage Δ
src/util/plotting.jl 91.30% <100.00%> (-1.29%) ⬇️

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 4423ebc...ab37628. Read the comment docs.

@devmotion
Copy link
Member

sampleplot added markers but made them so small that they only show up in the legend, not in the plot, which is kinda confusing, and I'd expect samples to generally be plotted on a fine grid where the markers don't make much sense anyways

I noticed that they show up in the example (which I thought was confusing...).

calling sampleplot with samples > 1 and a string as label would add one legend entry per sample, which quickly swamps the plot and isn't actually helpful - I've changed it so that if label is a string, it only adds a single legend entry for all the samples.

If you specify samples = n, layout = n you might actually want to have a label for each series/sample which would end up in different subplots. The example might be a bit constructed but it shows that this might not always be desired.

@st--
Copy link
Member Author

st-- commented Aug 24, 2021

If you specify samples = n, layout = n you might actually want to have a label for each series/sample which would end up in different subplots. The example might be a bit constructed but it shows that this might not always be desired.

I would argue that even in the samples=layout=n case it might be cleaner to have just one legend. But if you do want multiple labels, it's as easy as passing label=fill("samples", (1, n)), whereas the (in my opinion much more frequent use-case) of plotting ~100 samples and only wanting a single label seems rather clunkier...

@st--
Copy link
Member Author

st-- commented Aug 24, 2021

@devmotion I've added an explanation to the docstring, hopefully that makes things sufficiently clear for users who do want the original behaviour:) Just I found the previous behaviour of sampleplot(x, f; samples=100, label="samples") deeply confusing... (as a user, I pass in a single f, so I would expect to be able to pass in a single label to get a single legend entry) and it took me quite a bit to figure out how to fix it.

@st--
Copy link
Member Author

st-- commented Aug 24, 2021

I've now also added a use-case in the regression 1d example: )

Comment on lines 215 to 227
plt = plot(; xlim=(0, 1), xlabel="x", ylabel="y", title="posterior (AdvancedHMC)")
for (i, p) in enumerate(samples[(end - 100):end])
sampleplot!(
plt,
0:0.02:1,
gp_posterior(x_train, y_train, p);
samples=5,
label=(i == 1 ? "samples" : nothing),
)
end
scatter!(plt, x_test, y_test; label="Test Data")
scatter!(plt, x_train, y_train; label="Train Data", markercolor=1)
scatter!(plt, x_test, y_test; label="Test Data", markercolor=2)
plt
Copy link
Member Author

Choose a reason for hiding this comment

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

NB- is the explicit passing around of plt actually needed (/ recommended)? It would seem to me it should work exactly the same without it (the last scatter! also returns the plot object).

Copy link
Member

Choose a reason for hiding this comment

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

Not sure 🤷 I know that I had to use it sometimes since the plot was not returned from scatter! (or some other inplace function) but maybe it was fixed.

Comment on lines 216 to 226
for (i, p) in enumerate(samples[(end - 100):end])
sampleplot!(
plt,
0:0.02:1,
gp_posterior(x_train, y_train, p);
samples=5,
label=(i == 1 ? "samples" : nothing),
)
end
scatter!(plt, x_test, y_test; label="Test Data")
scatter!(plt, x_train, y_train; label="Train Data", markercolor=1)
scatter!(plt, x_test, y_test; label="Test Data", markercolor=2)
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like the only way of getting the scatter markers of train/test data on top of the lines for the samples is by having the call later. That then also affects the order of legend entries. I wish there was something straightforward like the z-order in matplotlib! If any of you has any ideas for how to get that behaviour, I'd love to find out (google didn't really help me here)..

Copy link
Member

Choose a reason for hiding this comment

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

No, I think you just have to plot in whatever order you want them to appear and then you have to live with the legend. At least I don't know of any other way.

@devmotion
Copy link
Member

Related: https://discourse.julialang.org/t/plots-only-one-series-as-primary-plot/24616/8

Apparently, the plots maintainers think it would be more natural to concatenate all data in a single vector separated with NaN 😬 Maybe we could add a keyword argument uniquelabels=true, as suggested in the discussion? I just think it is a bit unfortunate if we completely rule out the possibility to provide a string that should be duplicated for each plot as it is the current behaviour (BTW I have never managed to get a joint legend with subplots but maybe I didn't try long enough - the docs are quite difficult to navigate 😄 ). Since probably most users would want only a single label we could just set uniquelabels=true.

@st--
Copy link
Member Author

st-- commented Aug 24, 2021

I just think it is a bit unfortunate if we completely rule out the possibility to provide a string that should be duplicated for each plot as it is the current behaviour (BTW I have never managed to get a joint legend with subplots but maybe I didn't try long enough - the docs are quite difficult to navigate smile ). Since probably most users would want only a single label we could just set uniquelabels=true.

It's not impossible: does the explanation I added to the docstring, using label=fill("single label", (1, samples)), not satisfy you for that (rare?) use-case?

src/util/plotting.jl Outdated Show resolved Hide resolved
src/util/plotting.jl Outdated Show resolved Hide resolved
@devmotion
Copy link
Member

It's not impossible: does the explanation I added to the docstring, using label=fill("single label", (1, samples)), not satisfy you for that (rare?) use-case?

What I meant more precisely: it is not possible anymore if you pass a string as label even though in the Plots ecosystem usually that would give you the same label for each series. And I think label=fill("single label", (1, samples)) is more cumbersome than label="single label", uniquelabels=false since you have to use fill and pass the correct number of labels.

@st--
Copy link
Member Author

st-- commented Aug 24, 2021

in the Plots ecosystem usually that would give you the same label for each series.

Yes and that's what I'd expect if I pass in a series of fs! As a user, samples>1 doesn't give me any indication that it's suddenly multiple series behind the scenes (it might've just been samples connected with NaNs as you mentioned). But if it's gonna make you much happier I can add the uniquelabels.

@devmotion
Copy link
Member

My opinion is just:

  • If we treat the samples as multiple series, then we should not deviate too much from what is the standard for multiple series in the Plots.jl ecosystem. I think a uniquelabels keyword argument would be the most straightforward solution since it would still allow to supply a string as label but get a legend with multiple labels without having to construct the list of labels manually.
  • If we treat the samples as a single series, then we should concatenate them in a vector with the different samples separated by NaN. This is the only way to ensure that downstream transformations in the recipe pipeline treat the data as a single series. The label issue would be resolved automatically in this case.

I am leaning more and more towards the second option.

@st--
Copy link
Member Author

st-- commented Aug 24, 2021

I am leaning more and more towards the second option.

Sounds good 👍 I think that makes most sense conceptually as well. I'll have a go at that:)

Comment on lines 124 to 125
flat_x = vcat(Iterators.flatten(zip(Iterators.repeated(sp.x), fill(NaN, nsamples)))...)
flat_f = vcat(Iterators.flatten(zip(eachcol(samples), Iterators.repeated([NaN])))...)
Copy link
Member Author

Choose a reason for hiding this comment

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

@devmotion happy for you to suggest a better way - this is what I could come up with so far ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently the splatting is bad? but I can't figure out a way to get around it

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion would be

Suggested change
flat_x = vcat(Iterators.flatten(zip(Iterators.repeated(sp.x), fill(NaN, nsamples)))...)
flat_f = vcat(Iterators.flatten(zip(eachcol(samples), Iterators.repeated([NaN])))...)
flat_x = repeat(vcat(sp.x, NaN), nsamples)
flat_f = vec(vcat(samples, fill(NaN, 1, nsamples)))

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I literally just came upon the same solution myself right before looking at your comment 🎉

Copy link
Member Author

Choose a reason for hiding this comment

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

Except the "interleave with NaN" doesn't actually seem to work, as seriestype = :line makes it sort by x :(

So we could remove the seriestype --> :line below, but then you must pass in an ordered x for the sampleplot to make any sense. Would you be happy with that?

Copy link
Member

Choose a reason for hiding this comment

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

We could also just sort x, couldn't we?

src/util/plotting.jl Outdated Show resolved Hide resolved
st-- and others added 3 commits August 24, 2021 14:54
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@st--
Copy link
Member Author

st-- commented Aug 24, 2021

I think it makes more sense to treat the multiple samples as a single series, but I also don't think it needs to be sampleplot's responsibility to sort the x values. But I'm not sure what to do about the failing plotting tests - they seem to test some very specific implementation details (which doesn't strike me as particularly valuable tests); we could change them so we just test that the length of the "new" x is equal to samples * (length(x) + 1) ? or delete them? what do you prefer?

@devmotion
Copy link
Member

I'm happy if the tests are improved. I think in the tests we should check the plotting attributes that sampleplot defines. So I think it makes sense to try to check that x is correct, y looks reasonable, and the default values for other attributes correspond to what we define in sampleplot. As long as Plots does the correct thing even if the x values are not sorted, I think we should not sort them.

st-- and others added 2 commits August 25, 2021 11:58
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@st-- st-- requested a review from devmotion August 25, 2021 09:00
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Looks good, I just had some minor questions 🙂

0:0.02:1,
gp_posterior(x_train, y_train, p);
samples=5,
label=(i == 1 ? "samples" : nothing),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just

Suggested change
label=(i == 1 ? "samples" : nothing),
label=(i == 1 ? "samples" : ""),

? I did not know that label=nothing is a thing 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, one of the complaints about Plots.jl is that there are plenty of different ways of saying the same thing!
I find label = nothing to be a bit more self-explanatory, but feel free to add&commit your suggestion if you feel moderately strongly about it:)

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't feel strongly about it. The main reason would be to avoid having two different types here but I'm fine with nothing as well.

examples/regression-1d/script.jl Show resolved Hide resolved
Comment on lines 131 to 132
seriescolor --> "red"
label --> ""
Copy link
Member

Choose a reason for hiding this comment

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

Do we need/want these defaults?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I really like this default for label and would keep it. Otherwise it says y1 which is worse. If you want to get rid of the red for seriescolor, we can remove it here as well, but I'm also happy to leave it as is for now.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the default labels aren't great. Maybe let's just remove the series color? I don't think red is a particularly great choice and we could just let Plots pick one, according to whatever theme is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, done. anything else before you're willing to hit approve?:)

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Looks good to me, I think these changes are reasonable!

Edit: Should this be a breaking release?

@st--
Copy link
Member Author

st-- commented Aug 26, 2021

Edit: Should this be a breaking release?

Good point. Technically, yes (you might have relied on sampleplot plotting in red, or explictly wanted different samples as separate series with individual labels - or colors!), and whilst it kinda feels like the change is too small to warrant going from 0.4 to 0.5, I think it'd also be good practice for us to be consistent with the semantic versioning and not hold back changes just because they're not backwards-compatible. That's what 0.x is for, right?:)

@st--
Copy link
Member Author

st-- commented Aug 26, 2021

And in fact I did have to edit the examples to account for removing the seriescolor default, as it looked pretty bad with the random colors!

@devmotion
Copy link
Member

I agree. Maybe we could make a single breaking release with this PR and #216 to reduce the number of breaking releases? It feels though that we might still need time to think about and possibly improve the design in #216.

@devmotion
Copy link
Member

And in fact I did have to edit the examples to account for removing the seriescolor default, as it looked pretty bad with the random colors!

IMO it's reasonable that we have to set the seriescolor explicitly in this case - we call sampleplot repeatedly with different inputs, so technically these are different entities with possibly different labels and colors.

@st--
Copy link
Member Author

st-- commented Aug 26, 2021

to reduce the number of breaking releases?

I'd prefer to release this now - which comes at the cost of users having to bump their compat one additional time, but that's offset by smaller changes also being easier to adapt to. Otherwise we take the cost of more and more changes sticking around in un-merged PRs until it's "enough" to make a breaking release - and then a big breaking release actually becomes much more effort for users to adapt to.

What's crucial to minimise user pain is to make it easy for users to understand WHAT breaks in a breaking release (and how they might have to adjust their code to account for it). I'll update the PR description accordingly. Do we (want to) have guidelines around how to document breaking changes & make that clear in release notes? "[breaking]" in the PR title for example? that would make it easier to go to the release summary and focus on the breaking changes one by one.

@st-- st-- changed the title Plotting updates [breaking] change of sampleplot behaviour and defaults Aug 26, 2021
@st-- st-- changed the title [breaking] change of sampleplot behaviour and defaults **BREAKING** change of sampleplot behaviour and defaults Aug 26, 2021
@st-- st-- merged commit 7d42ab2 into master Aug 26, 2021
@st-- st-- deleted the st/sampleplot_update branch August 26, 2021 11:16
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