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

Add RecipesBase plotting recipe #340

Merged
merged 6 commits into from
Apr 24, 2021
Merged

Add RecipesBase plotting recipe #340

merged 6 commits into from
Apr 24, 2021

Conversation

nalimilan
Copy link
Member

This fixes plotting with Plots.jl, preserving the custom ordering of levels.

Fixes #256.

@daschw @mkborregaard Does this sound correct? I had to adjust the recipe you wrote at JuliaPlots/Plots.jl#2519 (comment) to support missing values. Since the recipe only has access to CategoricalValue entries and not the the parent CategoricalArray, I couldn't find a way to detect whether the array has a Union{<: CategoricalValue, Missing} eltype, so I always include missing in the set of possible values. But AFAICT plot automatically skips them when they are not present so I guess it's OK?

Also I've tried to write basic tests without relying on heavy checks of the produced graphs, let me know if there's a better way.

This fixes plotting with Plots.jl, preserving the custom ordering of levels.
@daschw
Copy link

daschw commented Apr 10, 2021

Hi, sorry for the late response! This looks good IMO. I tested on all backends if there are any issues with your missing hack, but it works fine. Unfortunately, I don't know of a better way to test recipes without producing graphs, but I guess your tests check everything the recipe does, so this should be fine.

@nalimilan
Copy link
Member Author

Thanks!

@nalimilan nalimilan merged commit 37ff301 into master Apr 24, 2021
@nalimilan nalimilan deleted the nl/plots branch April 24, 2021 10:31
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.

Integration with Plots.jl
2 participants