-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix to colouring for missing plots #45
Conversation
anesthetic/samples.py
Outdated
@@ -189,14 +193,15 @@ def plot_1d(self, axes, *args, **kwargs): | |||
Pandas array of axes objects | |||
|
|||
""" | |||
plot_type = kwargs.pop('types', 'kde') | |||
|
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.
So far I was using plot_type
(which is popped in samples.plot
) to decide what kind of 1D plot I want. Now, providing the kwarg plot_type
results in
TypeError: plot() got multiple values for keyword argument 'plot_type'
Code to reproduce:
pc = NestedSamples(root="./tests/example_data/pc")
pc.plot_1d(['x0', 'x1'], label="pc", plot_type='hist');
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.
Ah, I had noticed that MCMCSamples.plot_1d
was missing a 'types' argument, in analogy to MCMCSamples.plot_2d
. In this PR I've made them more consistent in preparation for the finer-grained control suggested in #41 .
The fix is to pass types='hist'
rather than plot_type='hist'
. I acknowledge that this is technically a breaking change, although only to undocumented (and, tbh unknown on my part) functionality.
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.
One of the things I'm considering is deprecating the cryptic types
arguments, with the plan to replace them with subplot_types
, which is more informative.
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.
By the way, the error actually also happens in the 2D case:
pc = NestedSamples(root="./tests/example_data/pc")
fig, axes = pc.plot_2d(['x0', 'x1', 'x4', 'x5'], label="pc", plot_type='kde');
Might it be more consistent to allow plot_type
as a kwarg for plot_2d
that broadcasts the provided string to all subplots?
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'm not sure how to fix this, but having plot_type
and types
in parallel like this seems wrong to me. If types
is the one intended to be passed to plot_1d
then plot_type
should not be a valid kwarg.
I acknowledge that this is technically a breaking change, although only to undocumented (and, tbh unknown on my part) functionality.
Not undocumented with #24. Actually, plot_type
was one of my major motivations for suggesting #24.
In this PR I've made them more consistent in preparation for the finer-grained control suggested in #41 .
The thing is, plot_type
and types
don't quite have the same role, so I'm not sure whether this is really more consistent. The need for types
arises because in the 2D case we distinguish between 3 different families of plots (diagonal, lower and upper) which all need a plot_type
assignment. This is a 2D specific feature and, thus, types
is unnecessary in the 1D case.
One of the things I'm considering is deprecating the cryptic types arguments, with the plan to replace them with subplot_types, which is more informative.
Replacing types
with a more general subplot_types
that determines e.g. which plot type for which parameter would be an option. At the same type plot_type
could be replaced with e.g. default_plot_type
that broadcasts the plot type across all subplots and subplot_types
can then be used to pick and alter specific parameters.
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.
In any case, might it be better to revert this specific change and address all this in a separate PR that targets #41 ?
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.
Agreed, reverted in 61e3f6b
Description
This PR added a failing test that shows the issue noted in #19. It now fixes that test by reordering the 'blank plots' in
MCMCSamples.plot
Fixes #19
Checklist:
flake8 anesthetic tests
)pydocstyle --convention=numpy anesthetic
)python -m pytest
)