Refactor plots2d #1317
Refactor plots2d #1317
Conversation
doc/source/CHANGELOG.rst
Outdated
| @@ -16,6 +16,8 @@ Changelog | |||
| - msm: fixed serialization of BayesianHMSM, if initialized with a ML-HMSM. #1283 | |||
| - base: require at least tqdm >= 4.23, because of an API change. #1292, #1293 | |||
| - plots: fixed minor bug in plot_network (state_labels=None would not work). #1306 | |||
| - plots: refactored plots2d | |||
There was a problem hiding this comment.
this message is kind of uninformative to the user. I would state, that the gca behaviour has changed etc.
pyemma/plots/plots2d.py
Outdated
| @@ -121,23 +114,27 @@ def plot_free_energy(xall, yall, weights=None, ax=None, nbins=100, ncountours=10 | |||
| weights : ndarray(T), default = None | |||
| sample weights. By default all samples have the same weight | |||
| ax : matplotlib Axes object, default = None | |||
| the axes to plot to. When set to None the default Axes object will be used. | |||
| the axes to plot to. When set to None the default Axes object | |||
| will be used. | |||
| nbins : int, default=100 | |||
| number of histogram bins used in each dimension | |||
| ncountours : int, default=100 | |||
| number of contours used | |||
| offset : float, default=0.1 | |||
pyemma/plots/plots2d.py
Outdated
| @@ -156,33 +153,403 @@ def plot_free_energy(xall, yall, weights=None, ax=None, nbins=100, ncountours=10 | |||
| ax : Axes object containing the plot | |||
|
|
|||
| """ | |||
| import matplotlib.pylab as _plt | |||
| #import matplotlib.pylab as _plt | |||
pyemma/plots/plots2d.py
Outdated
| logscale=logscale) | ||
|
|
||
|
|
||
| def plot_free_energy_new( |
There was a problem hiding this comment.
what is actually the difference to the old method? Can we decide to have only one or are they fundamentally different?
A short line in the docstring what this method does different would be nice.
There was a problem hiding this comment.
The new plotting functions always return fig, ax, cbar while the old functions return either fig, ax or just ax. Further, plot_free_energy has a parameter typo (countour) which has been fixed in the new function.
I would love to immediately replace everything with the new functions but this would slightly break the API.
There was a problem hiding this comment.
We could have a legacy switch parameter to steer the old or new behaviour, but this would convolute the code a bit. I think we should deprecate the old function and promote the new one, shouldn't we?
There was a problem hiding this comment.
Yes, indeed. I don't think functions would be too convoluted, though. Using a legacy switch is a good idea!
There was a problem hiding this comment.
Do we need to conserve the sequence of named parameters in legacy mode?
Codecov Report
@@ Coverage Diff @@
## devel #1317 +/- ##
=========================================
+ Coverage 91.64% 92.05% +0.4%
=========================================
Files 223 224 +1
Lines 24756 26095 +1339
=========================================
+ Hits 22687 24021 +1334
- Misses 2069 2074 +5
Continue to review full report at Codecov.
|
…ot_scatter_contour function
|
Do we need to conserve the sequence of named parameters in legacy mode?
strictly speaking yes, because if somebody put in the parameters in the
previous order, you would break the intended usage of the parameter.
|
|
For |
Adjust levels to be equally fine in both cases
|
@cwehmeyer we discussed that it is potentially useful to have access to colormap or mappable. Since I found it a bit inconsistent to return either the colormap or the mappable, depending on the kwargs, they are now written into a dictionary and returned together. Otherwise, larger customized plots might break down if somebody chooses to alter some of the plot options. I'm not completely happy with it though; if you have a better idea feel free to implement it. |
|
Thx @cwehmeyer. Can you add a line about the kwargs (i.e. what function they are passed to) to the docstrings? |
|
@thempel: done. |
This PR refactors the plots2d module: