Curve analysis drawer refactoring #738
Conversation
nkanazawa1989
changed the title
| # Set figure size if axis is not provided. | ||
| # Otherwise user may set the figure size by oneself. | ||
| # Then avoid applying draw defaults to avoid override user's preference. |
There was a problem hiding this comment.
This is a bit confusing. Do we need this comment? The code looks clean enough.
There was a problem hiding this comment.
I moved the comment to option description. Indeed this should not be a comment for developers.
434b894
| # Add inset axis. User may provide a single axis object via the analysis option, | ||
| # while this analysis tries to draw its result in multiple canvases, | ||
| # especially when the analysis consists of multiple curves. | ||
| # Inset axis is experimental implementation of matplotlib 3.0 so maybe unstable API. |
There was a problem hiding this comment.
Is this a good idea if the API is unstable?
There was a problem hiding this comment.
This already exists in the current codebase and this is unavoidable. I investigated the Matplotlib but there is no approach other than this to add inset axes. Since this is not user-facing API in our codebase, we can update the code without any deprecation when something is deprecated in Matplotlib.
|
|
||
|
|
||
| class CurveDrawerMixin: | ||
| """Mixin to provide drawing functionalities for curve fit result.""" |
There was a problem hiding this comment.
Can you add a few more details regarding this mix-in? I.e. what drawing functionalities does it provide? When is it called in a curve analysis etc...
There was a problem hiding this comment.
We should also mention what sub-classes need to implement. As far as I can tell it looks like they need to implement:
def _default_draw_options(cls):correct?
There was a problem hiding this comment.
Yes, I'll update the docs after we finalize the design. I'm still wondering mixin vs instance (see my long reply above). You are correct, what the subclass may want to implement is _default_draw_options. But nothing stops you to override other methods (if you need fancy visualization).
There was a problem hiding this comment.
With this change 09a16bf _default_draw_options has been removed. Now developer needs to directly update drawer instance option via analysis options.
qiskit_experiments/curve_analysis/standard_analysis/error_amplification_analysis.py
Show resolved
Hide resolved
- move default analysis options of Rabi experiment to constructor - lint updates
nkanazawa1989
force-pushed
the
curve_analysis_refactor/drawer
branch
2 times, most recently
from
1f0f976 to
09a16bf
Compare
nkanazawa1989
force-pushed
the
curve_analysis_refactor/drawer
branch
from
f834cd4 to
680a0d2
Compare
nkanazawa1989
changed the title
|
|
||
| This method should implement a protocol to initialize a drawing canvas | ||
| with user input ``axis`` object. Note that curve analysis drawer | ||
| supports visualization in the 2D inset axes. This method |
There was a problem hiding this comment.
What is 2D inset axis? Can you define it?
There was a problem hiding this comment.
Done in bcd8a6e
|
|
||
| A subclass must implement following abstract methods. | ||
|
|
||
| initialize_canvas |
There was a problem hiding this comment.
The text below seems very precise. Why can this not be done in the base class? Because you want subclasses to be able to init axis with different sizes and shapes?
There was a problem hiding this comment.
No, subclass is not necessary based on the Matplotlib. For example, plotly will give you cool interactive visualization of experimental results. Someone may want to implement.
| Note that figure and axis might be different plot when a user provide | ||
| an axis object which is a part of other multiple axis figure. |
There was a problem hiding this comment.
This is a bit hard to follow. Can you reformulate?
There was a problem hiding this comment.
Done in bcd8a6e. This is too specific comment for Matplotlib so I moved it to the mpl drawer rather than in the base drawer.
| draw_ops = { | ||
| "markersize": 9, | ||
| "zorder": 5, | ||
| "linestyle": "-", | ||
| "linewidth": 2, | ||
| } |
There was a problem hiding this comment.
Is there any way to configure these values?
There was a problem hiding this comment.
Yes. In follow-up you can add arbitrary drawing options to override them. This is indeed one of feedback from the beta-test.
Co-authored-by: Daniel J. Egger <[email protected]>
Co-authored-by: Daniel J. Egger <[email protected]>
Co-authored-by: Daniel J. Egger <[email protected]>
Co-authored-by: Daniel J. Egger <[email protected]>
Co-authored-by: Daniel J. Egger <[email protected]>
|
These are the figures. Basically the same as before. T1 CR Hamiltonian (multi-canvas) |
Co-authored-by: Daniel J. Egger <[email protected]>
This PR replaces existing curve drawer with JSON serializable new drawer instance. - New drawer has dedicated to options to simplify the curve analysis options. - New drawer is stateful (axis object is internally kept after initialized) so that we can draw different data immediately. - Drawer code can be easily tracked by IDE. Co-authored-by: Daniel J. Egger <[email protected]>
Summary
This is part of #265
Details and comments
Curve drawer functions are replaced with a mixin class
CurveDrawerMixin. With this change, we can draw data at arbitrary timing, thus this is a first step to drop__processed_datafrom curve analysis (with RB refactoring we don't need to keep raw data). In addition, analysis options for fitting and drawing are organized as separate options.