Warn on invalid value for log scale based series #3601

t-bltg · 2021-07-03T12:09:04Z

With this PR, the example in #3594 now shows a warning or a full stacktrace controlled via the JULIA_DEBUG environment variable.

@mortenpi, is this what you expect ?

~~If someone has a tip for logging the messages only once, I'm all ears ...~~
==> now warning only once
~~Not sure about the potential performance penalty traversing series though.~~
==> now traversing only once

Fix #3594.

$ julia bug.jl
┌ Warning: Invalid negative or zero value 0.0 found at series index 3 for log10 based yscale
└ @ Plots [...]/Plots/Bfn5f/src/utils.jl:92

$ JULIA_DEBUG=Plots julia bug.jl
┌ Warning: Invalid negative or zero value 0.0 found at series index 3 for log10 based yscale
└ @ Plots [...]/Bfn5f/src/utils.jl:92
┌ Debug: 
│   exception =
│    DomainError with 0.0:
│    
│    Stacktrace:
│      [1] macro expansion
│        @ ./logging.jl:341 [inlined]
│      [2] series_segments(series::Plots.Series, seriestype::Symbol; check::Bool)
│        @ Plots [...]/Bfn5f/src/utils.jl:93
│      [3] pgfx_add_series!(#unused#::Val{:path}, axis::PGFPlotsX.Axis, series_opt::PGFPlotsX.Options, series::Plots.Series, series_func::Type{PGFPlotsX.Plot}, opt::RecipesPipeline.DefaultsDict)
│        @ Plots [...]/Bfn5f/src/backends/pgfplotsx.jl:334
│      [4] (::Plots.PGFPlotsXPlot)(plt::Plots.Plot{Plots.PGFPlotsXBackend})
│        @ Plots [...]/Bfn5f/src/backends/pgfplotsx.jl:287
│      [5] _update_plot_object(plt::Plots.Plot{Plots.PGFPlotsXBackend})
│        @ Plots [...]/Bfn5f/src/backends/pgfplotsx.jl:1415
│      [6] prepare_output(plt::Plots.Plot{Plots.PGFPlotsXBackend})
│        @ Plots [...]/Bfn5f/src/plot.jl:245
│      [7] show
│        @ [...]/Bfn5f/src/output.jl:214 [inlined]
│      [8] png(plt::Plots.Plot{Plots.PGFPlotsXBackend}, fn::String)
│        @ Plots [...]/Bfn5f/src/output.jl:7
│      [9] png(fn::String)
│        @ Plots [...]/Bfn5f/src/output.jl:10
│     [10] main(i::Int64)
│        @ Main [...]/bug.jl:10
│     [11] top-level scope
│        @ [...]/bug.jl:16
│     [12] include(mod::Module, _path::String)
│        @ Base ./Base.jl:386
│     [13] exec_options(opts::Base.JLOptions)
│        @ Base ./client.jl:285
│     [14] _start()
│        @ Base ./client.jl:485
└ @ Plots [...]/Bfn5f/src/utils.jl:93

mortenpi

This is awesome -- thanks for looking into this! I think it's a great UX improvement.

However, I don't think that we should sanitize the data: it is the responsibility of the user not to feed invalid data (since again this is undefined behavior, and fixing undef is arbitrary and thus subject to debate).

The GR warning still showing up would be my only minor complaint, but I do see your point. And since are printing the bigger warning anyway, I think it's fine.

My only concern would be that passing broken data on to the backend may break it. I do manage to break GR on a regular basis (as in, independent follow-up plots in the same session fail to work render, and I have to either change backend or restart Julia).

On another note, for some reason, for the original MWE it does print the new warning three times:

julia> plot(
           1:5, [1,2,0,4,5],
           m = [:utriangle, :dtriangle, :utriangle, :dtriangle, :dtriangle],
           yaxis = :log10, ylims = (1e-1, 1e1),
       )
┌ Warning: Invalid negative or zero value 0.0 found at serie index 3 for log10 based yscale
└ @ Plots ~/.julia/packages/Plots/7T4MM/src/utils.jl:92
GKS: Number of points is invalid in routine POLYLINE
┌ Warning: Invalid negative or zero value 0.0 found at serie index 3 for log10 based yscale
└ @ Plots ~/.julia/packages/Plots/7T4MM/src/utils.jl:92
┌ Warning: Invalid negative or zero value 0.0 found at serie index 3 for log10 based yscale
└ @ Plots ~/.julia/packages/Plots/7T4MM/src/utils.jl:92

It's in the REPL.. so perhaps it is because when it opens the GKS QtTerm window, it renders it multiple times or something along those lines?

src/utils.jl

t-bltg · 2021-07-04T10:42:20Z

This is awesome -- thanks for looking into this! I think it's a great UX improvement.

Thanks for the feedback !

The GR warning still showing up would be my only minor complaint, but I do see your point. And since are printing the bigger warning anyway, I think it's fine.

Dully noted.

My only concern would be that passing broken data on to the backend may break it. I do manage to break GR on a regular basis (as in, independent follow-up plots in the same session fail to work render, and I have to either change backend or restart Julia).

In the end the plots appears or is saved to file, so this doesn't "break" GR in the running julia session, does it ?

On another note, for some reason, for the original MWE it does print the new warning three times:

Yes, I've seen that. By inspecting the new debug traces, I found why it happened and pushed a fix. Can you try with the latest commit ? On my machine I now get:

$ julia bug.jl
┌ Warning: Invalid negative or zero value 0.0 found at series index 3 for log10 based yscale
└ @ Plots [...]/Plots/Bfn5f/src/utils.jl:92
GKS: Number of points is invalid in routine POLYLINE

t-bltg · 2021-07-04T14:47:42Z

~~We hit JuliaStats/Distributions.jl#1358.~~

Fixed.

codecov · 2021-07-04T18:07:59Z

Codecov Report

Merging #3601 (0247f18) into master (06c551b) will increase coverage by 0.00%.
The diff coverage is 83.33%.

❗ Current head 0247f18 differs from pull request most recent head 124d2d6. Consider uploading reports for the commit 124d2d6 to get more accurate results

@@           Coverage Diff           @@
##           master    #3601   +/-   ##
=======================================
  Coverage   65.09%   65.09%           
=======================================
  Files          27       27           
  Lines        6660     6672   +12     
=======================================
+ Hits         4335     4343    +8     
- Misses       2325     2329    +4

Impacted Files	Coverage Δ
src/utils.jl	`52.63% <60.00%> (+0.04%)`	⬆️
src/backends/gr.jl	`89.00% <94.73%> (+0.02%)`	⬆️
src/backends/pgfplotsx.jl	`60.94% <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 0ca5bc9...124d2d6. Read the comment docs.

mortenpi · 2021-07-04T22:48:20Z

Yes, I've seen that. By inspecting the new debug traces, I found why it happened and pushed a fix. Can you try with the latest commit ?

Yup, can confirm that it's now down to just one warning!

julia> include("mwe.jl")
  Activating environment at `~/Julia/test/plots-number-of-points/Project.toml`
┌ Warning: Invalid negative or zero value 0.0 found at series index 3 for log10 based yscale
└ @ Plots ~/.julia/packages/Plots/empMW/src/utils.jl:92
GKS: Number of points is invalid in routine POLYLINE

In the end the plots appears or is saved to file, so this doesn't "break" GR in the running julia session, does it ?

Ah, I did not mean that this particular case breaks GR. Even if GR emits the GKS: Number of points is invalid in routine POLYLINE, it looks like this has no effect on follow-up plots (or even the current plot).

However, in the past I have managed to break GR to the point where even follow-up plots fail to render properly by (accidentally) passing bad data to plot. So I was thinking that, as a general principle, it might not be a good idea to pass on bad data to the backend. However, I don't have an MWE at hand though, and it's always possible that I was running into some one-off bug that has already been fixed.

warn on invalid value for log scale based series

3536acb

t-bltg force-pushed the warn_log branch from 925a766 to 3536acb Compare July 3, 2021 12:47

mortenpi reviewed Jul 3, 2021

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

issue warning only once - apply to other backends

124d2d6

t-bltg force-pushed the warn_log branch from dae0787 to 124d2d6 Compare July 4, 2021 13:48

t-bltg closed this Jul 4, 2021

t-bltg reopened this Jul 4, 2021

t-bltg requested a review from BeastyBlacksmith July 5, 2021 09:10

BeastyBlacksmith approved these changes Jul 5, 2021

View reviewed changes

t-bltg closed this Jul 5, 2021

t-bltg reopened this Jul 5, 2021

t-bltg merged commit 0122d70 into JuliaPlots:master Jul 5, 2021

t-bltg deleted the warn_log branch July 5, 2021 10:13

t-bltg mentioned this pull request Jul 6, 2021

[BUG] Bad values with yscale=:log10 break follow-up plots in GR #3321

Closed

mcabbott mentioned this pull request Dec 5, 2021

[BUG] GR error message when y=0 and yaxis=:log10 #3594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn on invalid value for log scale based series #3601

Warn on invalid value for log scale based series #3601

t-bltg commented Jul 3, 2021 •

edited

Loading

mortenpi left a comment

t-bltg commented Jul 4, 2021 •

edited

Loading

t-bltg commented Jul 4, 2021 •

edited

Loading

codecov bot commented Jul 4, 2021 •

edited

Loading

mortenpi commented Jul 4, 2021

Warn on invalid value for log scale based series #3601

Warn on invalid value for log scale based series #3601

Conversation

t-bltg commented Jul 3, 2021 • edited Loading

mortenpi left a comment

Choose a reason for hiding this comment

t-bltg commented Jul 4, 2021 • edited Loading

t-bltg commented Jul 4, 2021 • edited Loading

codecov bot commented Jul 4, 2021 • edited Loading

Codecov Report

mortenpi commented Jul 4, 2021

t-bltg commented Jul 3, 2021 •

edited

Loading

t-bltg commented Jul 4, 2021 •

edited

Loading

t-bltg commented Jul 4, 2021 •

edited

Loading

codecov bot commented Jul 4, 2021 •

edited

Loading