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

Allow passing collection of tuples to series_annotations #3743

Merged
merged 3 commits into from
Aug 3, 2021

Conversation

t-bltg
Copy link
Member

@t-bltg t-bltg commented Aug 2, 2021

Fix #3629. And some code cleanup.

Example

using Plots
plot([1, 2], [3, 4], series_annotations=[("A", :red), ("B", :blue)])
plot([1, 2], [3, 4], series_annotations=(["A", "B"], [:red, :blue]))  # also supported
plot([1, 2], [3, 4], series_annotations=[["A", "B"], [:red, :blue]])  # same as previous

Output

rel1

@t-bltg t-bltg force-pushed the ann branch 25 times, most recently from 610b251 to 78d1bb3 Compare August 2, 2021 21:36
@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #3743 (c11fc9f) into master (2df85eb) will increase coverage by 0.06%.
The diff coverage is 71.42%.

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

@@            Coverage Diff             @@
##           master    #3743      +/-   ##
==========================================
+ Coverage   62.97%   63.03%   +0.06%     
==========================================
  Files          28       28              
  Lines        7063     7045      -18     
==========================================
- Hits         4448     4441       -7     
+ Misses       2615     2604      -11     
Impacted Files Coverage Δ
src/backends.jl 60.52% <0.00%> (-0.54%) ⬇️
src/examples.jl 61.76% <ø> (ø)
src/components.jl 68.22% <71.56%> (+1.64%) ⬆️
src/args.jl 73.18% <100.00%> (ø)

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 2df85eb...39cb596. Read the comment docs.

@mkborregaard
Copy link
Member

mkborregaard commented Aug 3, 2021

Given that this is series-annotations, would it be make sense to allow passing of a tuple of equal-length vectors, e.g.

plot([1, 2], [3, 4], series_annotations=(["A", "B"], [:red, :blue]))

src/components.jl Outdated Show resolved Hide resolved
@t-bltg
Copy link
Member Author

t-bltg commented Aug 3, 2021

Given that this is series-annotations, would it be make sense to allow passing of a tuple of equal-length vectors, e.g.

plot([1, 2], [3, 4], series_annotations=(["A", "B"], [:red, :blue]))

I find this less readable than using plot(..., series_annotations=[("A", :red), ("B", :blue)]).

@t-bltg
Copy link
Member Author

t-bltg commented Aug 3, 2021

That PR was unreadable sorry, I've split it into two commits, the first one relative to the fix of the issue, and the second relative to code cleanup. @BeastyBlacksmith, I've removed the deprecated directed_curve as well.

@mkborregaard
Copy link
Member

mkborregaard commented Aug 3, 2021

find this less readable than using

Yes sure. But most of the time you'll be plotting data from tables, and they will have that format, e.g. plot(height, shoes, seriesannotations = (names, textcolor). For series_annotations(as opposed to annotations) It's very rare to type them out in practice, because you'd tend to have more than 2-3 points. I guess accepting a zip is an easy workaround.

@t-bltg
Copy link
Member Author

t-bltg commented Aug 3, 2021

Yes sure. But most of the time you'll be plotting data from tables, and they will have that format, e.g. plot(height, shoes, seriesannotations = (names, textcolor). For series_annotations(as opposed to annotations) It's very rare to type them out in practice, because you'd tend to have more than 2-3 points. I guess accepting a zip is an easy workaround.

Ha, I think I misunderstood your first comment.
I've implemented what you expected (see the updated example), 🤞 for not breaking the CI.

@t-bltg t-bltg changed the title Allow passing tuple to series_annotations Allow passing collection of tuples to series_annotations Aug 3, 2021
@t-bltg t-bltg merged commit 40b5df3 into JuliaPlots:master Aug 3, 2021
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.

[FR] Allow tuples of Plots.text arguments to series_annotations
3 participants