search

JuliaPlots / RecipesPipeline.jl

Utilities for processing recipes
http://juliaplots.org/RecipesPipeline.jl/dev/
MIT License
17 stars 17 forks source link

Test for error value from `apply_recipe` fallback. #95

Closed BioTurboNick closed 3 years ago

BioTurboNick commented 3 years ago

For compatibility with https://github.com/JuliaPlots/Plots.jl/pull/3765

Catching an exception here as part of the normal flow can vastly slow down plotting, as it happens frequently. Returning an error value instead, which the calling function can check, can speed up plotting 2x.

Addresses #93

t-bltg commented 3 years ago

Looks like our CI is broken, I'm trying to fix that.

In the mean time, looks like some failures are related to this PR: https://github.com/JuliaPlots/RecipesPipeline.jl/pull/95/checks?check_run_id=3426189759#step:8:515.

t-bltg commented 3 years ago

CI is back, re-running checks.

BioTurboNick commented 3 years ago

I believe these errors are only happening because the compensatory change in Plots.jl hasn't happened. The exception is still being thrown from inside Plots.jl, which must be swallowed upstream somewhere, leading to a fallback of some sort.

So, I restored the try/catch block but kept the check for nothing. I added a comment that the block can be removed once Plots.jl makes the compensatory change.

BeastyBlacksmith commented 3 years ago

I always wanted to get rid of that, thanks for digging in!

BioTurboNick commented 3 years ago

Great, thanks all! I'd like to note on the Plots.jl PR what release this will likely be in. Would it likely be v0.3.5, since it isn't breaking? Or are there other breaking changes in the works that would make it v0.4?

t-bltg commented 3 years ago

Would that be ok if we wait for https://github.com/JuliaPlots/RecipesPipeline.jl/pull/94 to be finalized / merged before bumping to 0.3.5 ?

Changed my mind about that, let's finalize https://github.com/JuliaPlots/Plots.jl/pull/3765.

t-bltg commented 3 years ago

@BioTurboNick, https://github.com/JuliaPlots/Plots.jl/pull/3765 is emrge: when https://github.com/JuliaRegistries/General/pull/43601 will be merged, you can make a new PR to remove the try/catch part (per https://github.com/JuliaPlots/RecipesPipeline.jl/pull/95#issuecomment-905983156).