Skip to content

Conversation

@leespen1
Copy link
Contributor

@leespen1 leespen1 commented Sep 1, 2022

Changed function gr_add_legend to call gr_draw_marker instead of gr_draw_markers, since a legend entry only has one marker (if any).

Fix #3996

I explained the issue and solution in greater detail in this post: https://discourse.julialang.org/t/fixing-missing-marker-in-plot-legend-when-first-value-in-series-is-nan/86658

In summary, the example

using Plots; gr()
plot([NaN,2,3], markershape=:circle)

produced a graph with no marker in the legend:
NaN_legend_marker_issue

This only happens when the first entry in the series is NaN. If only the second entry is NaN, we get:

using Plots; gr()
plot([1,NaN,3], markershape=:circle)

NaN_not_first_legend_marker

Morever, this behavior does not happen for the PlotlyJS backend (I did not test backends besides GR and PlotlyJS):
he second entry is NaN, we get:

using Plots; plotlyjs()
plot([NaN,2,3], markershape=:circle)

NaN_legend_marker_plotlyjs

By changing the gr_draw_markers call to a gr_draw_marker call, this behavior was fixed. Now the example

using Plots; gr()
plot([NaN,2,3], markershape=:circle)

produces the following graph:
NaN_legend_marker_fixed

And it works for a single series with multiple marker shapes as well:

using Plots; gr()
plot([1,2,3], markershape=[:circle, :square, :diamond])

NaN_legend_marker_fixed_multiple_labels

The only downside I can see is the following example:

using Plots; gr()
plot([NaN,2,3], markershape=[:circle, :square, :diamond])

NaN_legend_marker_fixed_multiple_labels_first_NaN

The marker in the legend is a circle, although no circle appears in the series because the first entry in the series is not plotted (because it's NaN). Still, the previous behavior was to have no marker at all, which I think is worse.

…`gr_draw_markers`, since a legend entry only has one marker (if any).
@t-bltg t-bltg added the GR label Sep 1, 2022
@t-bltg
Copy link
Member

t-bltg commented Sep 1, 2022

You might want to read https://docs.juliaplots.org/stable/contributing/#Write-code,-and-format.

Also, please rebase your PR against current master for CI to run without errors.

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Base: 80.23% // Head: 81.05% // Increases project coverage by +0.81% 🎉

Coverage data is based on head (c6af026) compared to base (3f8904a).
Patch coverage: 89.43% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4328      +/-   ##
==========================================
+ Coverage   80.23%   81.05%   +0.81%     
==========================================
  Files          29       28       -1     
  Lines        7009     7316     +307     
==========================================
+ Hits         5624     5930     +306     
- Misses       1385     1386       +1     
Impacted Files Coverage Δ
src/Plots.jl 87.80% <ø> (ø)
src/recipes.jl 67.42% <0.00%> (+1.35%) ⬆️
src/output.jl 62.38% <33.33%> (-0.58%) ⬇️
src/utils.jl 75.62% <66.66%> (+0.29%) ⬆️
src/layouts.jl 79.77% <75.00%> (+3.07%) ⬆️
src/types.jl 88.67% <85.00%> (+6.86%) ⬆️
src/backends/unicodeplots.jl 87.55% <90.90%> (+2.07%) ⬆️
src/backends/gr.jl 89.02% <96.42%> (+0.44%) ⬆️
src/args.jl 80.39% <100.00%> (+0.72%) ⬆️
src/axes.jl 85.83% <100.00%> (-0.60%) ⬇️
... and 28 more

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.

@leespen1
Copy link
Contributor Author

leespen1 commented Sep 8, 2022

Every check passed except "ci / Julia 1.6 - macos-latest (pull_request)". Does this mean the fix (or the changes made to the original project) is not compatible with Julia1.6 on MacOS? I use Ubuntu, so I'm not sure what I should do about this.

@t-bltg
Copy link
Member

t-bltg commented Sep 8, 2022

This is just a spurious random error related to network failure : GitError(Code:ERROR, Class:Net, SecureTransport error: connection closed via error).

I'm re-running the failed job now, don't worry this is not blocking.

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 think that this PR is well explained (here and on discourse), and I see no regressions in the examples on ci, so I don't see any reason why this shouldn't go in.

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.

Thank you for this fix! You might want to check out #3503.

@t-bltg t-bltg added the bug label Sep 9, 2022
@BeastyBlacksmith BeastyBlacksmith merged commit f9e48b5 into JuliaPlots:master Sep 9, 2022
@leespen1
Copy link
Contributor Author

You're welcome, thanks for the review!

leespen1 added a commit to leespen1/Plots.jl that referenced this pull request Sep 10, 2022
Adding name to contributor list after first bug fix accepted (JuliaPlots#4328 (comment)).
@leespen1 leespen1 mentioned this pull request Sep 10, 2022
t-bltg pushed a commit that referenced this pull request Sep 11, 2022
Adding name to contributor list after first bug fix accepted (#4328 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Legend marker does not appear when first y value is NaN on scatter

3 participants