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

[BUG] Cycling arguments #2980

Open
briochemc opened this issue Sep 16, 2020 · 8 comments
Open

[BUG] Cycling arguments #2980

briochemc opened this issue Sep 16, 2020 · 8 comments

Comments

@briochemc
Copy link
Collaborator

briochemc commented Sep 16, 2020

Plots.jl cycling arguments (but only sometimes) makes for a confusing experience. With Plots.jl v1.6.4, depending the backend,

scatter(1:10, 1:3)

either:

  • plots by repeating 1:3 with, e.g., GR and PyPlot:
    gr pyplot

  • plots only the first 3 points with, e.g., UnicodePlots:
    unicodeplots

  • or errors with, e.g., Plotly:

    julia> plotly()
    Plots.PlotlyBackend()
    
    julia> scatter(1:10, 1:3)
    Error showing value of type Plots.Plot{Plots.PlotlyBackend}:
    ERROR: BoundsError: attempt to access 3-element UnitRange{Int64} at index [1:10]

I think it should either always error, or at least throw a warning.

Related issue: Note I read that

Plots is intended to cycle input arguments, this allows you to do things like scatter(1:4, [1]). But it's a good question why the backends are inconsistent! Plotlyjs has a third behaviour - it plots (1:100, 1:100). I hope you're OK with my renaming of the issue.

Originally posted by @mkborregaard in #1151 (comment)

But I think think this cycling of arguments is not a good idea. (My opinion being biased by having been sent into a 3-day rabbit-hole expedition because of that "feature".)

While I understand the appeal of being able to do things like scatter(1:4, [1]), I think it exposes the risk that someone (like me) mistakenly plots scatter(x,y) with x and y of different lengths, and cannot figure out what is happening. This is true especially for large-ish datasets that produce a somehow dense cloud of data. I would personally prefer the behavior to be only valid when calling a scalar, i.e. for scatter(1:4, 1) to actually do what scatter(1:4, [1]) currently does, but I think that may conflict with other things (e.g., what is discussed in #2129).

Backends

This bug occurs on ( insert x below )

Backend yes no untested
gr (default) x
pyplot x
plotly x
plotlyjs x
pgfplotsx x
inspectdr x
unicodeplots x

Versions

  • Plots.jl version: v1.6.4
  • Backend version (]st -m):
    • GR v0.52.0
    • PyPlot v2.9.0
    • UnicodePlots v1.3.0
  • Output of versioninfo():
    julia> versioninfo()
    Julia Version 1.5.1
    Commit 697e782ab8 (2020-08-25 20:08 UTC)
    Platform Info:
      OS: macOS (x86_64-apple-darwin19.5.0)
      CPU: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
      WORD_SIZE: 64
      LIBM: libopenlibm
      LLVM: libLLVM-9.0.1 (ORCJIT, skylake)

Edit: Fixed the MWE (1:10 instead of 1:20)

@mkborregaard
Copy link
Member

FWIW I've changed my mind on this, and agree it's unfortunate.

@BeastyBlacksmith
Copy link
Member

The question is, what behaviour do we want? Personally, I like the zip-like behaviour of the unicode backend.

@briochemc
Copy link
Collaborator Author

I don't know what you will decide, but IMHO if it does not error, I really think it should throw a warning!

@mkborregaard
Copy link
Member

I don't know what the unicode backend does - but wouldn't it be most consistent to error but have a recipe for scatter(1:10, Iterators.cycle(1:3))?

@BeastyBlacksmith BeastyBlacksmith added this to the v2.0 milestone Sep 16, 2020
@briochemc
Copy link
Collaborator Author

briochemc commented Sep 17, 2020

I don't know what the unicode backend does - but wouldn't it be most consistent to error but have a recipe for scatter(1:10, Iterators.cycle(1:3))?

I showed what the output of UnicodePlots up there! With it, scatter(1:10, 1:3) plots the first 3 points only (the "missing" points are not plotted, but the xlim covers (1,10)). Here it is again:

@mkborregaard
Copy link
Member

Oh sorry! Yeah I can't think of a situation when that would be the desired behaviour.

@yha
Copy link
Member

yha commented Sep 22, 2020

I don't know what the unicode backend does - but wouldn't it be most consistent to error but have a recipe for scatter(1:10, Iterators.cycle(1:3))?

I agree that erroring would be best, and if we do this breaking change, I would extend it to other cases where we currently cycle, which seems to be some pseudo-random backend-dependent subset of plots attributes taking vector values. Would be nice if we can abolish Plots._cycle.
Maybe we can directly support arbitrary iterables, so things like scatter(1:10, Iterators.cycle(1:3)) can work?
I think these look quite nice:

using Iterators: repeated, cycle
plot(cycle([0,1]), y)
scatter(x, repeated(0))
plot(y, color=cycle(1,2), fillrange=repeated(0))
# each column against the same vector:
plot(repeated(x), eachcol(Y))
# or maybe
plot(repeated(x), Y)

I think this can work by zipping and collecting the positional inputs, and only then proceeding to dispatch by types according to the recipe system. Just need to error early if none of the inputs has known finite length (based on Base.IteratorSize).

@t-bltg
Copy link
Member

t-bltg commented Nov 28, 2022

This weird "un-official, un-documented, scatter only" cyclic behavior should be fixed removed in 2.0.

As of the scatter(1:4, [1]) thing, supporting Iterators from Base in a recipe as suggested in #2980 (comment) would be acceptable to me.

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

No branches or pull requests

5 participants