Skip to content

Conversation

conradhaupt
Copy link
Contributor

@conradhaupt conradhaupt commented Sep 8, 2022

Summary

The following PR contains code to improve the plotting functionality of Qiskit Experiments. There are two big changes: 1) moving the visualization submodule of curve_analysis to its own submodule and 2) the creation of new plotter and drawer classes which managed plotting figures and interfacing with plotting backends respectively.

Details and Comments

The original BaseCurveDrawer class from qiskit_experiments.curve_analysis.visualization was used by CurveAnalysis to draw scatter points, curve-fits, and fit-reports. However, this limited usage of BaseCurveDrawer in other parts of Qiskit Experiments and for figures other than a curve-fit, such as IQ plots.

The new module (qiskit_experiments.visualization) contains two new submodules plotters and drawers, which are the core of this PR. All other code from qiskit_experiments.curve_analysis.visualization, that hasn't been modified much, has also been moved to the new submodule. Plotters and drawers fulfill different functions, such that analysis classes interact with a plotter which intern interacts with a drawer. A drawer implements a generic interface (BasePlotter) for a plotting backend, such as Matplotlib. A plotter deals with managing data to be plotted and invoking specific drawing functions of a drawer. CurveAnalysis now contains a plotter instead of a drawer.

image

BasePlotter implements an interface to set plotting data and generate a figure. Subclasses of BasePlotter override _plot_figure(), where drawing methods are called on self.drawer (i.e, drawer.draw_*). Implemented subclasses of BasePlotter and BaseDrawer are CurvePlotter and MplDrawer respectively. You can refer to MplDrawer and CurvePlotter for example usage and implementations of these base-classes. There are three important stages to plotting with this new module:

  1. Setting data for plotting (done by an analysis class or user).
  2. Setting plotter and drawer options.
  3. Generating and returning the figure (for the plotter, this involves calling drawer draw_* functions).

Setting data for plotting

Plotters separate data into two groups: series and figure data. Series data is a nested dictionary where the first key is a series (or curve) name (called series_name) and the second key (called data_key) identifies the data associated with the series. Figure data is also a dictionary, but only of depth-one, also indexed by a data_key. Series data contains data-values to be plotted that are related to a given series, curve, or fit model. A series can be seen as a single entry in a legend. Figure data contains data-values or information related to the entire plot/figure; such as a fit report or text to be drawn on the canvas. Helper functions exist in BasePlotter to assist subclasses with querying and setting series and figure data: data_for, data_exists_for, data_keys_for, set_series_data, set_figure_data. A plotter subclass defines the data-keys that are accepted in expected_series_data_keys() and expected_figure_data_keys(), but only to raise a warning to users when set_series_data and set_figure_data are called. Unexpected data-keys are still added to the internal data storage.

Setting options

Both BasePlotter and BaseDrawer have two sets of options: normal options (<plotter/drawer>.options) and figure-options (<plotter/drawer>.figure_options). Normal options, called drawer and plotter options in the two classes, contain parameters that control how the classes function. Examples include the subplots tuple which defines the number of sub-axes to use and the initial axis object. For CurvePlotter, an additional option is plot_sigma which is used when drawing confidence intervals. Figure-options are options that define what is plotted/drawn on the canvas. This includes the axis labels, axis units, and figure title. The distinction goes further in that the first step taken, when a plotter generates a figure, is to pass its figure-options to plotter.drawer. The figure-options passed to the drawer are the intersection between plotter.figure_options and plotter.drawer.figure_options, where the plotter has priority. For example,

plotter.set_figure_options(x=0, y=1)
plotter.drawer.set_figure_options(x="a", z="b")
# After plotter.figure() called.
opts = plotter.drawer.figure_options
opts.x	# returns 0, not "a"
opts.y	# returns 1
opts.z	# raise an exception as z doesn't exist in drawer.figure_options

By separating options from figure-options, BasePlotter can control which options overwrite their counterpart in plotter.drawer. A user of BasePlotter and its subclasses is aware (after reading the documentation) that plotter.figure_options contains information that is drawn onto the canvas. Advanced users would be aware that drawer.figure_options is updated by the plotter. By keeping the two separate --- and updating the drawer when generating a figure --- a user, or class isn't forced to interact with the nested drawer instance.
As well as splitting options into two sets, this PR introduces a new style class PlotStyle to contain style parameters such as axis-label text size and the default figure size. There was a legacy class PlotterStyle used by fit_result_plotters.py, though the code didn't appear to be used anywhere in Qiskit Experiments. PlotStyle takes inspiration from PlotterStyle but instead subclasses Options, with new methods to merge and update instances. BaseDrawer has two style instances, a default style (an option) and custom style (a figure-option), where the full style for a drawer instance is their combined style (see BaseDrawer.style). BasePlotter only has one style instance (an option) as the drawing backend is more likely to define extra style parameters. plotter.drawer.figure_options.custom_style is assigned the value of plotter.options.style when plotter.figure() is called, and thus a user does not need to interact with it.

Generating and returning the figure

Subclasses of BasePlotter represent a type of figure, such as a curve-fit (CurvePlotter) or IQ plot. The core functions to be overridden in such a subclass are _default_options, _default_figure_options, and _plot_figure. The last of the three functions is called by BasePlotter.figure(), and is where calls to plotter.drawer.draw_* should be made. BaseDrawer contains the same, or equivalent, drawing functions as BaseCurveDrawer, from before this PR.

Given that the figure is only generated when accessed (i.e., when calling plotter.figure()), we delay drawing onto the canvas until all relevant data has been loaded into the plotter, via the data assignment functions. The original procedure for generating a figure using BaseCurveDrawer was to initialize the canvas, draw, then format the canvas. This procedure is kept, but is now contained in BasePlotter.figure(). The object returned is the same object returned by plotter.drawer.figure.

Tests

  • I have implemented end-to-end tests for BasePlotter, BaseDrawer, and MplDrawer. There are also tests for the new PlotStyle class and whether plotter.options and plotter.figure_options are passed to plotter.drawer correctly.
  • I have verified that the following experiments still generate the same figures with the new visualization module. Note that there was a bug with the legend-location style option before this PR, which is now fixed. This is why legends may be located differently.

CrossResonanceHamiltonian

image

T2Ramsey

image

QubitSpectroscopy

image

StandardRB

image

Questions for Reviewers

  • In refactoring and moving qiskit_experiments.curve_analysis.visualization, I noticed that fit_result_plotters.py was not used anywhere in QE. Should it be removed?
  • I am not 100% sure how to ensure options and plot-options are added to the generated documentation. It looks like there are custom styles for experiment and analysis classes, but it may be overboard to do this for the plotter and drawer functionality.

Still To Do

  • Complete serialization of BasePlotter and BaseDrawer. (implemented in c40594c)
    • plotter data is not currently serialized. I don't think it should be as it isn't needed after figures have been generated and the data exists in the experiment data anyway.
  • Add release notes.

@conradhaupt conradhaupt marked this pull request as draft September 8, 2022 07:11
@conradhaupt conradhaupt changed the title Move curve visualization to elevated submodule [WIP] Move curve visualization to elevated submodule Sep 8, 2022
@conradhaupt
Copy link
Contributor Author

@eggerdj and @nkanazawa1989, it would be great if you could review this PR. Let me know if you have any questions about the implementation.

@conradhaupt conradhaupt marked this pull request as ready for review September 16, 2022 08:43
@conradhaupt conradhaupt changed the title [WIP] Move curve visualization to elevated submodule [WIP] Add new plotting module for experiment and analysis figures Sep 16, 2022
Copy link
Contributor

@eggerdj eggerdj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are a few points on the first few files.

Copy link
Contributor

@eggerdj eggerdj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks overall good to me. The structure looks fine. I have a few minor nitpicks.

@conradhaupt conradhaupt changed the title [WIP] Add new plotting module for experiment and analysis figures Add new plotting module for experiment and analysis figures Sep 21, 2022
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @conradhaupt , this is really great start of designing a unified visualization framework for QE. Especially I like the separation of plotter and drawer, which allows us to easily switch visualization backend. Indeed, not only backend itself, but also we can modify the appearance output image from the same backend by providing user-defined drawer.

My concern is mainly about flexibility. For example, if we want to draw extra axhline (in terms of matplotlib), we need to define custom data structure in the plotter and write new drawing method for drawer. Namely such analysis class should define both plotter and drawer subclass. This is because current methods are designed based off of the context of visualization, while other popular visualization packages define data structure based on abstraction of visualized object. I think such pattern is not always perfect, but worth revisiting the data structure so that an experiment author can prepare plotter instance for non-curve analysis type visualization (or some extended one) with minimum overhead.

Apart from flexibility, this PR actually includes several breaking API changes. Most critical one is the module path since the experiment Json loader identifies the class by import path. In this sense, I would leave conventional drawer as-is with addition of deprecation warning, and replaces the default plotter in existing analysis classes with new one (this also prevents breaking the user code). I think it is also necessary to write upgrade release note which would include a mini-migration guide for developers.

@@ -20,7 +20,7 @@
import numpy as np
import uncertainties
from qiskit_experiments.exceptions import AnalysisError
from qiskit_experiments.curve_analysis import plot_scatter, plot_errorbar
from qiskit_experiments.visualization import plot_scatter, plot_errorbar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still doesn't use new plotter framework. Likely this is due to proposed implementation tied too much to curve analysis, and new framework is likely overkill for such simple drawing (just plotting scatter and line, no distinction between series and global data).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this analysis class has not been migrated to the new framework yet. The goal is to migrate this to use a plotter, but most likely this will happen in a follow-up PR.

"""Final cleanup for the canvas appearance."""

@abstractmethod
def draw_raw_data(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these methods are too much optimized to curve analysis. My original implementation doesn't assume to be general visualization framework at all, and thus I heavily optimized it for curve analysis with context-oriented methods like this. However, currently you are generalizing it, and I think function-oriented naming would make the base class more flexible. For example, if you implement

  • draw_scatter
  • draw_line
  • draw_filled_area
  • draw_text

existing methods can be reimplemented by

  • draw_raw_data = draw_scatter (without error bar data)
  • draw_formatted_data = draw_scatter (with error bar data)
  • draw_line = draw_line
  • draw_confidence_interval = draw_filled_area
  • draw_report = draw_text

with some tweak of data model. I would reimplement QV analysis visualization with new framework without defining QVPlotter -- requiring subclass may hesitate experiment authors to use new framework. This may give you more idea for generalized visualization framework. Another example would be figure3a of this paper (T1 landscape with Stark tone). We don't have any fit model for this curve, and we just want to draw scatter with broken line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree with renaming the drawer functions to be more generic. I will go ahead and make these changes.

Regarding implementing QVPlotter or not: one of the goals of this framework is that a BasePlotter subclass defines a kind of figure being plotted. This can be a curve-fit/X-Y plot or an IQ level-1 plot. Obviously, this can be changed slightly so that maybe a given subclass is applicable to CurveAnalysis and QV analysis. I can see that there may be additional drawer functions that would allow the module to recreate figure3a, but I'll leave those additional functions for a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed BaseDrawer function names in 94158f4.

displayed in the scientific notation.
yval_unit (str): Unit of y values. See ``xval_unit`` for details.
figure_title (str): Title of the figure. Defaults to None, i.e. nothing is shown.
series_params (Dict[str, Dict[str, Any]]): A dictionary of plot parameters for each series.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to me like subclass option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think series_param should be part of BaseDrawer as it defines backend-/drawer-independent properties of different series. If it was a subclass option, then BasePlotter would need a different way of defining series-specific properties, such as their colour and axis/canvas index, which would then be passed to different drawer subclasses in subclass specific ways.

:meth:`figure` when :attr:`drawer` can be used to draw on the canvas.
"""

def figure(self) -> Any:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic looks nice 💯

"""Returns the expected figures data-keys supported by this plotter."""

@property
def options(self) -> Options:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

option and plot_option are confusing and I cannot intuitively understand difference without reading the documentation. What about renaming option to figure or canvas option (I guess this is what you want to have)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely understand that. @eggerdj suggested drawer_options or options_for_drawer, which are also good. In hindsight, it would be good for users to be oblivious of the drawer unless otherwise necessary. If that's the case, maybe the options should be renamed to options and figure_options; where figure_options are options/parameters for the figure itself (like axis labels etc.) whereas options are parameters that control the behaviour of the plotter or drawer. I've made this change in 956696a, but we can continue discussing it here.

from qiskit_experiments.framework import Options


class PlotStyle(Options):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this cannot be a standard dictionary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each BaseDrawer subclass must define a default style, which should inherit from the parent drawer. This allows users to only define style-parameters they want to overwrite. The default style of a drawer subclass is defined in _default_style(), allowing subclasses to add extra style parameters easily. This is a similar mechanism to options, plot_options, and experiment_options. To be consistent with other _default_* functions, the style type would have to have an interface similar to Options. To combine the default style with the custom style, defined by a user, the style type also needs to allow merging and updating; exactly like a dictionary. It was easier to extend Options --- and add merge() and update() --- than subclass dict and add functionality equivalent to that provided by Options. This inheritance also means PlotStyle benefits from value validation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any problem of promoting this to plotter options. Anyways the nested option structure makes it hard to be managed. As far as I can see,

- plotter.options
     +- some_option1
     +- some_option2
     +- style (PlotStyle)
            +- figsize
     ...
- plotter.figure_options
     +- some_option1
     ...
- plotter.drawer.options
     +- some_option1  

I think plotter.options and figure_options are okey, and they provide clean structure. However, if we want to add more hierarchy, it's probably better to introduce something like namespace, such as

plotter.options.style.foo1
plotter.options.axis.x.bar1
plotter.options.axis.y.bar1
plotter.options.series.bar2
plotter.options.general.foo2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree that nested Options are actually bad. I checked, and Options doesn't support "namespaced" options being set unless it's as a dictionary key (opts["key-name"] = ...), which limits setting them using plotter.options.style.foo1 = "boo". Having a dedicated class --- instead of merging styles with options --- also allows BaseDrawer to separate the default style parameters from custom style parameters, so that users can view only those options which have changed. This is the reason I added default_style and custom_style options. The same functionality could be achieved usingoptions and a set of altered options, but then it wouldn't be clear which options are style parameters and which aren't as the names may not be clear; unless we use namespaced names (which is difficult with Options).

Given other comments also bring into question whether PlotStyle should subclass Options, I have refactored the PlotStyle class to inherit from dict (see 7c3ca90 and 8ed2c9f). I have kept style parameters separate from options and figure_options be keeping the default_style and custom_style options. This allows BaseDrawer subclasses to merge and update style parameters (see BaseDrawer.style) while keeping them in a manageable container. Regarding namespacing style parameters, the fullstop in variable names makes it difficult to use PlotStyle. For example, style parameters cannot be set in __init__ as kwargs (i.e., PlotStyle(tick.text_size=14) doesn't work). Because of this, I've left the style parameter names with underscores instead of fullstops. I do think the namespaced/grouped interface would be useful, and should be considered as the visualization module is extended.

Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @conradhaupt . The updates in the previous commits are pretty nice. I'm happy with new classes. The relationship between plotter and drawer is now much clear. Basically, we

  • subclass plotter depending on the purpose of figure
  • subclass drawer for different drawing backend

and new drawer methods are sufficiently agnostic to the plotter. I added some more comments mainly about option hierarchy.

@@ -355,10 +391,8 @@ def _run_analysis(
for group, fit_data in fit_dataset.items():
chisqs.append(r"reduced-$\chi^2$ = " + f"{fit_data.reduced_chisq: .4g} ({group})")
report += "\n".join(chisqs)
self.drawer.draw_fit_report(description=report)
self.plotter.set_figure_data(report_text=report)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively can we delegate the generation of text data to CurvePlotter? This allows us to much flexibly generate report by subclassing the plotter. We can do in follow-up, but likely such change causes API break? For example, when we run analysis with many curves, the report will occupy huge portion of the canvas and we cannot see any curve. In this case we may want to drop chi-squared. I think we can set List[AnalysisResultData] to figure data and let the curve plotter generate default report with capability of hooking. This doesn't block merging, just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the idea of passing a list of AnalysisResultData instances to the plotter. To keep this PR from exploding, I think we should address this in a follow-up PR. But it's definitely something I think will be useful. I'll open an issue about this once this PR has been merged.

subplots=(3, 1),
style=PlotStyle(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's difficult to infer the behavior of this option from this code (i.e. this is passed to drawers). To me (from user perspective) it looks like PlotStyle ~ plotter.options and cannot find why they are wrapped by a class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping style parameters in a class, or another container, helps to group them for later processing and portability. Subclasses of BaseDrawer need to be able to 1) set default style parameters which are subclass compatible and 2) have plotters or users override certain style parameters. This could be accomplished as standard options, but then it would be confusing as to if an option is a style parameter or drawer option that modifies behaviour. Matplotlib uses rcparams, Qiskit Terra, and the old curve_analysis.visualization module uses a dedicated class. However, I agree with your other comment that PlotStyle should rather be a dictionary to avoid nested Options instances.

Copy link
Contributor Author

@conradhaupt conradhaupt Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've converted PlotStyle to inherit from dict instead of Options in 7c3ca90.

from qiskit_experiments.framework import Options


class PlotStyle(Options):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any problem of promoting this to plotter options. Anyways the nested option structure makes it hard to be managed. As far as I can see,

- plotter.options
     +- some_option1
     +- some_option2
     +- style (PlotStyle)
            +- figsize
     ...
- plotter.figure_options
     +- some_option1
     ...
- plotter.drawer.options
     +- some_option1  

I think plotter.options and figure_options are okey, and they provide clean structure. However, if we want to add more hierarchy, it's probably better to introduce something like namespace, such as

plotter.options.style.foo1
plotter.options.axis.x.bar1
plotter.options.axis.y.bar1
plotter.options.series.bar2
plotter.options.general.foo2

new.text_box_rel_pos: Tuple[float, float] = (0.6, 0.95)

# size of fit report text
new.text_box_text_size: int = 14
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only valid option for curve plotter although this class is designed to be generic. perhaps same for text_box_rel_pos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This style parameter is needed if we want draw_text_box in BaseDrawer. The comment was somewhat outdated as the parameter is not just for fit-reports, but I've fixed that.


BaseDrawer
MplDrawer
LegacyCurveCompatDrawer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is instantiated only when a user provides the legacy curve drawer in analysis options? This seems to me like an internal class and no public API docs is necessary unless end-users directly deal with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. It is only used for backwards compatibility. Addressed in 6bdc6ef.

ind = self._series.index(name) % len(self.DefaultMarkers)
return self.DefaultMarkers[ind]

def _update_label_in_dict(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _update_label_in_dict(
def _update_label_in_options(

I think this name is clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Addressed in b604350.

drawer: The drawer to use when creating the figure.
"""
# Data to be plotted, such as scatter points, interpolated fits, and confidence intervals
self._series_data: Dict[str, Dict[str, Any]] = {}
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 Sep 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about supplementary_data? Because all experimental data should belong to series_data according to your description, thus this will be the field to store non-experimental data.

self._figure_options.update_options(**fields)
self._set_figure_options = self._set_figure_options.union(fields)

def _initialize_drawer(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really initialize the drawer (calling initialize_canvas). What about _mirror_options? Of course calling the initialize_canvas here should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name in the doc string says it all. How about def _configure_drawer or maybe even better def _prepare_drawer()?

Copy link
Contributor Author

@conradhaupt conradhaupt Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with _configure_drawer as that matches the docstring, which I feel describes what the function does. Addressed in 84ef573.

Copy link
Contributor

@eggerdj eggerdj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have minor comments. Overall this looks good.

self._figure_options.update_options(**fields)
self._set_figure_options = self._set_figure_options.union(fields)

def _initialize_drawer(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name in the doc string says it all. How about def _configure_drawer or maybe even better def _prepare_drawer()?

Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @conradhaupt for the awesome work! Now this PR looks good to me. Really excited to use new drawer (and much cool one with interactive backend).

@conradhaupt conradhaupt requested a review from eggerdj October 3, 2022 14:24
@nkanazawa1989 nkanazawa1989 added the on hold On hold until something else is done. label Oct 3, 2022
Copy link
Contributor

@eggerdj eggerdj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nkanazawa1989 nkanazawa1989 removed the on hold On hold until something else is done. label Oct 5, 2022
Copy link
Contributor

@eggerdj eggerdj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the naming update.

@eggerdj eggerdj merged commit cd6f92a into qiskit-community:main Oct 6, 2022
wshanks pushed a commit to thaddeus-pellegrini/qiskit-experiments that referenced this pull request Oct 14, 2022
…ommunity#902)

* The visualization code has been moved to its own module. It now also implements a bridge interface 
where a plotter determines what data is plotted and how while a drawer implements a plotting backend
(such as matplotlib).

Co-authored-by: Daniel J. Egger <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
@conradhaupt conradhaupt mentioned this pull request Oct 18, 2022
eggerdj added a commit that referenced this pull request Nov 22, 2022
…ule to docs. (#961)

* This PR expands on/updates plotters to add options for controlling some features and updating style parameters to better handle more use-cases. A secondary addition of this PR, which was missing from #902, is to ensure the visualization module is added to the documentation.

Co-authored-by: Daniel J. Egger <[email protected]>
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.

3 participants