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

add non-array version for hline and vline #2796

Closed
wants to merge 1 commit into from

Conversation

daschw
Copy link
Member

@daschw daschw commented Jun 17, 2020

Not sure if we want this but this would be an easy workaround only for the hline/vline shorthands for #2129 and #2575.

allows e.g.

plt = plot(rand(10))
hline!(0.5)
vline!(plt, 7)

I am really sceptical about this, because

plot(1, st = :hline)

does something different from

hline(1)

@BeastyBlacksmith
Copy link
Member

I don't think

plot(1, st = :hline)

is often seen in the wild. I would be in favor of this change.

@PhilipVinc
Copy link
Contributor

I'm nobody,
but I'd love this change. It always bites me that it does not work.

@daschw
Copy link
Member Author

daschw commented Jun 18, 2020

@mkborregaard You seemed to have some doubts about this in #2575. What's your opinion on this?

@mkborregaard
Copy link
Member

I agree with your scepticism. I generally don't like when we make workarounds that distort our ability to reason about things, so plot(4, st = :hline) should be equivalent to hline(4).
If we really want this, I think we should deprecate the plot(4) functionality, which is poorly documented anyway. I tend to use it actually, to create a plot with 4 subplots and plot to them iteratively, but I'd be OK with changing the interface to plot(num_subplots = 4), as it is actually a perversion of Plots logic itself that plot(4) passes a positional parameter that isn't the data. Breaking of course, but seems harmless.

@daschw
Copy link
Member Author

daschw commented Jun 18, 2020

I'd also prefer a more general approach without special-casing different series types. However, hline and vline are special cases anyway regarding the positional input arguments. hline(x, y) and vline(x, y) would both ignore x and for vline y would correspond to the x axis.

I tend to use it actually, to create a plot with 4 subplots and plot to them iteratively, but I'd be OK with changing the interface to plot(num_subplots = 4)

Can't you just do plot(layout = 4) or do you want to push! to initialized series?

@isentropic
Copy link
Member

Perhaps array versions could be named vlines, hlines?

@mkborregaard
Copy link
Member

mkborregaard commented Jun 19, 2020

Can't you just do plot(layout = 4) or do you want to push! to initialized series?

🤯

I guess we really could just deprecate plot(4) then. But would you then make passing a scalar automatically wrap that in a 1-lenght array? It just feels so... R-like.

@yha
Copy link
Member

yha commented Jun 25, 2020

I'm in favor of deprecating the current meaning of plot(::Int) and promoting scalars to 1-length vectors regardless of series type. I think this can be done pretty easily in a recipe.
I think this is a very natural interpretation in scatter(x,y) as well as hline/vline, and it seems in line with the Plots approach of providing a flexible intuitive interface with many ways of doing the same thing. Seems harmless to me, especially compared to other ways Plots already tries to guess the user's intention, like the different handling of vectors-of-vectors versus vectors-of-tuples.

I tend to use it actually, to create a plot with 4 subplots and plot to them iteratively, but I'd be OK with changing the interface to plot(num_subplots = 4)

But plot(4) currently creates 4 series in one subplot, not 4 subplots...

@daschw daschw closed this Jun 30, 2020
@daschw daschw deleted the hvline branch June 30, 2020 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants