-
Notifications
You must be signed in to change notification settings - Fork 31
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 plotting via Makie as Extension #454
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #454 +/- ##
==========================================
- Coverage 84.70% 81.90% -2.80%
==========================================
Files 20 21 +1
Lines 1072 1111 +39
==========================================
+ Hits 908 910 +2
- Misses 164 201 +37 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach is a bit too opinionated and inflexible. Could we instead define convert_arguments
(to tell Makie how it should view and use chains) and - if a new type of plots is needed - add a plot recipe (see https://docs.makie.org/dev/explanations/recipes/)?
After talking with the Makie devs on Discord:
The latest commit is the recommended approach, including registering an error hint if a Makie-related package isn't yet loaded and |
@devmotion / @theogf pinging since you had some initial feedback requesting a recipes-based approach, but I don't think that works when wanted to plot multiple chains/parameters (see my last post). If good with the overall approach here, I can knock out the remaining To-Dos. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plots you are proposing look good, but the problem is that it is too restrictive/opiniated for being in a package like MCMCChains.jl. If convert_arguments
is not sufficient, you can use the @recipe
macro no?
https://docs.makie.org/stable/explanations/recipes/#full_recipes_with_the_recipe_macro
ext/MCMCChainsMakieExt.jl
Outdated
import Makie | ||
|
||
|
||
function MCMCChains.trace(chns::T) where {T<:MCMCChains.Chains} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think trace
is an appropriate name, it should be something like traceplot
or plot_chains
to make it clear this is outputting a figure.
If you decide to go the method dispatch way, it would be good to deconstruct it a bit better:
- Make a mutable function that takes an
Axis
- Create separate functions for each subplot.
This way the code is beneficial for anyone who would just like some part of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think trace is an appropriate name, it should be something like traceplot or plot_chains to make it clear this is outputting a figure.
trace
follows the convention that plot names in the Julia ecosystem don't have the plot
postfix, e.g. scatter
or lines
instead of scatterplot
or linesplot
If convert_arguments is not sufficient, you can use the @recipe macro no?
Recipes are for creating a single sub-plot, not defining a figure with multiple sub-plots. I'm not sure there's a way to use a recipe unless all chains and parameters were plotted into a single sub-plot and not faceted the way MCMC plots tend to be
Make a mutable function that takes an Axis
Edit: I think I understand the desire for this, but I think this goes back to the fact that MCMC-type plots are facets of multiple plots/axes instead. MCMC-type plots are inherently figures not just axes/plots So, e.g. trace!(axis,...)
isn't feasible with the facets.
Create separate functions for each subplot.
i.e. where you could get the left side and right sight separately?
ext/MCMCChainsMakieExt.jl
Outdated
n_samples = length(chns) | ||
n_params = length(params) | ||
|
||
colors = Makie.to_colormap(:tol_vibrant) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @devmotion this is a bit too opiniated, what if I want to use a different set of colors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will explore ways to make more flexible.
ext/MCMCChainsMakieExt.jl
Outdated
width = 600 | ||
height = max(400, 80 * n_params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this, it is too restrictive...
Current Status:
A color-blind-friendly color scheme (
:tol_vibrant
) is used to plot the trance and parameter densities for each parameter in each chain.Sample plots:
Basic Layout
Large number of parameters
** Large number of chains**
To-do
Extend(not good idea)Makie.plot
instead of a newmyplot
functionSample Script