Conform iplot_histogram and plot_histogram #866
Conversation
mtreinish
requested review from
ajavadia,
atilag,
delapuente,
diego-plan9,
ewinston and
nonhermitian
as code owners
This commit fixes the inconsistencies between the 2 histogram plot functions iplot_histogram (for an interactive js plot) and plot_histogram (for a matplotlib generated plot). Both functions perform the same function just using different tools. To make them interchangeable this commit addresses the differences between the 2 and makes the interface the same between the 2. However, because of backwards compat concerns with plot_histogram (which has been present in several releases already) there is a some duplication of number_to_keep kwarg and the options dict. This deprecates the number_to_keep parameter so users can migrate off of it and then we can remove it from both plot_histogram and iplot_histogram. With the interfaces to both functions being the same this commit also updates the __init__ module to leverage iplot_histogram by default if qiskit.tools.visualization.plot_histogram() is called and we're running under jupyter and have a network connection to download the necessary js. Fixes Qiskit#864
mtreinish
force-pushed
the
plot-histogram-conform
branch
from
bea039d to
39ca802
Compare
|
this was fast :-) |
There was a problem hiding this comment.
Thank you for this addition, I think it's great to have a homogeneous API for visualizations.
Can you also add a couple of entries in the changelog to refer to this change and the other introduced in #862?
| @@ -17,35 +17,68 @@ | |||
| import matplotlib.pyplot as plt | |||
|
|
|||
|
|
|||
| def plot_histogram(data, number_to_keep=False): | |||
| def plot_histogram(data, number_to_keep=False, legend=None, options=None): | |||
There was a problem hiding this comment.
Can we emit deprecation warnings for number_to_keep? Change the signature of the function to:
def plot_histogram(data, legend=None, options=None, **kwargs):And check if number_to_keep has been passed. If so, emit a deprecation warning:
if 'numbers_to_keep' in kwargs:
warnings.warn('number_to_keep has been deprecated, use ...' DeprecationWarning)
numbers_to_keep = kwargs.get('numbers_to_keep', False)There was a problem hiding this comment.
I don't want to change the signature here, because the order of named kwargs actually matters. For example someone could be using this like: plot_histogram(data_input, 5) and that would work fine but if I move numbers_to_keep those users will be broken. But, I'll add the deprecation warning
| if isinstance(data, dict): | ||
| data = [data] | ||
| if legend and len(legend) != 1: | ||
| raise Exception("Length of legendL %s doesn't match number of " |
There was a problem hiding this comment.
We try to use functionality-specific errors, subclassing QiskitError. I think a VisualizationError class would make sense here. Do you mind to create this new class of errors and pass through other raise statements and change them accordingly?
| ax.text(rect.get_x() + rect.get_width() / 2., 1.05 * height, | ||
| '%f' % float(height), | ||
| ha='center', va='bottom') | ||
| for item, execution in enumerate(data): |
There was a problem hiding this comment.
Non-blocking: do you think is it feasible or even useful to allow customization of graph width, graph height and sort to equate the options dictionary passed to this function to that supported by the iplot_function?
There was a problem hiding this comment.
It should be, and I thought about that when I was refactoring things. But I didn't want to change things too much here. After this merges I'll work on updating this to add the missing options.
| single bar called other values | ||
| Args: | ||
| data (list or dict): This is either a list of dictionaries containing: | ||
| values to represent (ex {'001': 130}) |
There was a problem hiding this comment.
can you complete this sentence: "either ... or"?
This commit fixes several issue related to the plot_histogram function. Specifically it creates a new VisualizationError Exception class for errors raised during execution, now properly emits a DeprecationWarning when the deprecated number_to_keep kwarg is used, and updateds unclear docstrings.
This commit updates the changelog to document the recent changes to the visualization module.
This comimt clenas up the pyllint errors in the newly added visualization._error and visualization._counts_visualization modules.
mtreinish
force-pushed
the
plot-histogram-conform
branch
from
1ffeb0e to
4a1ddb6
Compare
This commit fixes the inconsistencies between the 2 histogram plot functions iplot_histogram (for an interactive js plot) and plot_histogram (for a matplotlib generated plot). Both functions perform the same function just using different tools. To make them interchangeable this commit addresses the differences between the 2 and makes the interface the same between the 2. However, because of backwards compat concerns with plot_histogram (which has been present in several releases already) there is a some duplication of number_to_keep kwarg and the options dict. This deprecates the number_to_keep parameter so users can migrate off of it and then we can remove it from both plot_histogram and iplot_histogram. With the interfaces to both functions being the same this commit also updates the __init__ module to leverage iplot_histogram by default if qiskit.tools.visualization.plot_histogram() is called and we're running under jupyter and have a network connection to download the necessary js. It also creates a new VisualizationError Exception class for errors raised during execution, now properly emits a DeprecationWarning when the deprecated number_to_keep kwarg is used, and updateds unclear docstrings. Fixes Qiskit#864
Summary
This commit fixes the inconsistencies between the 2 histogram plot
functions iplot_histogram (for an interactive js plot) and
plot_histogram (for a matplotlib generated plot). Both functions perform
the same function just using different tools. To make them
interchangeable this commit addresses the differences between the 2 and
makes the interface the same between the 2. However, because of
backwards compat concerns with plot_histogram (which has been present in
several releases already) there is a some duplication of number_to_keep
kwarg and the options dict. This deprecates the number_to_keep parameter
so users can migrate off of it and then we can remove it from both
plot_histogram and iplot_histogram.
With the interfaces to both functions being the same this commit also
updates the init module to leverage iplot_histogram by default if
qiskit.tools.visualization.plot_histogram() is called and we're running
under jupyter and have a network connection to download the necessary
js.
Details and comments
Fixes #864