Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

Test for error value from apply_recipe fallback. #95

Merged
merged 3 commits into from
Aug 26, 2021

Conversation

BioTurboNick
Copy link
Member

For compatibility with JuliaPlots/Plots.jl#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 t-bltg self-requested a review August 25, 2021 20:45
@t-bltg
Copy link
Member

t-bltg commented Aug 25, 2021

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
Copy link
Member

t-bltg commented Aug 25, 2021

CI is back, re-running checks.

@BioTurboNick
Copy link
Member Author

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
Copy link
Member

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

@t-bltg t-bltg merged commit 2cab220 into JuliaPlots:master Aug 26, 2021
@BioTurboNick
Copy link
Member Author

BioTurboNick commented Aug 26, 2021

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
Copy link
Member

t-bltg commented Aug 26, 2021

Would that be ok if we wait for #94 to be finalized / merged before bumping to 0.3.5 ?

Changed my mind about that, let's finalize JuliaPlots/Plots.jl#3765.

@t-bltg
Copy link
Member

t-bltg commented Aug 26, 2021

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

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

Successfully merging this pull request may close these issues.

3 participants