CurveAnalysis base class #765

nkanazawa1989 · 2022-04-06T20:26:36Z

Summary

This PR introduces base class for curve analysis. See #737 for details.

~~Blocked by #762~~

Details and comments

TODO

Add developer reno
Cleanup unit tests
Cleanup api documents (port documentation upgrade from Introducing group to curve analysis #715 )
Update subclasses

eggerdj

This seems to be in good shape. Do you also need to refactor the other analysis classes to this?

qiskit_experiments/curve_analysis/base_curve_analysis.py

eggerdj · 2022-04-21T08:27:03Z

qiskit_experiments/curve_analysis/base_curve_analysis.py

+        A method to format curve data. By default this method takes y value average
+        over the same x values and then sort the entire data by x values.


Suggested change

A method to format curve data. By default this method takes y value average

over the same x values and then sort the entire data by x values.

A method to format curve data. By default this method takes the average of the y values

over the same x values and then sort the entire data by x values.

This is not something that is needed all the time right? This is most useful for RB correct? Perhaps that's worth mentioning here.

No this is always done. Because some guess function relies on the sequence ordering, e.g. computing slope by something like dx = x[1] - x[0]. There is no guarantee x values are in ascending order.

There are two things here.

a) averaging the y data
b) sorting according to x data.

Makes sense to always do b). Do we always do a)?

Yes, should do. If you concatenate two experiments, you will create experiment data that may consist of something like x = [0,1,2,3,0,1,2,3] that will crash some guess function.

qiskit_experiments/curve_analysis/base_curve_analysis.py

eggerdj · 2022-04-21T08:58:59Z

qiskit_experiments/curve_analysis/curve_analysis.py

-        https://github.com/Qiskit/qiskit-experiments/issues
-    """
+class CurveAnalysis(BaseCurveAnalysis):
+    """Base class for curve analyis."""


This is not a base class is it?

This is still base class (e.g. ErrorAmplificationAnalysis(CurveAnalysis)). The reason we introduce BaseCurveAnalysis is to remove boilerplate code in GroupedCurveAnalysis.

hmmm the naming gets a bit confusing no? Since CurveAnalysis is still a base class it could be called BaseCurveAnalysis which is already taken. What are the distinctive features between CurveAnalysis (a base class) and BaseCurveAnalysis? Perhaps that can help with the naming.

Can you also add a bit more to the class docstring to explain why we have this class? Currently it reads Base class for curve analyis. which is too brief given that we have another class called BaseCurveAnalysis.

This will be clearer once we introduce the GroupCurveAnalysis (perhaps MultiGroupCurveAnalysis to be precise). The base one is the superclass of both CurveAnalysis and GroupCurveAnalysis . The difference between CurveAnalysis and BaseCurveAnalysis is implementation of _run_analysis method. In the group one the logic inside the _run_analysis will be different from CurveAnalysis though each subroutines can be reused. This is why BaseCurveAnalysis only implements subroutines.

qiskit_experiments/curve_analysis/curve_data.py

qiskit_experiments/library/characterization/analysis/drag_analysis.py

…ure/curve_analysis_baseclass

Co-authored-by: Daniel J. Egger <[email protected]>

- small docs fix - lint - reno

eggerdj

I think this looks mostly good. The only thing I still find a bit awkward is the naming between CurveAnalysis and BaseCurveAnalysis as both are base classes (see some of the comments below).

eggerdj · 2022-04-25T15:18:17Z

qiskit_experiments/curve_analysis/curve_analysis.py

-        https://github.com/Qiskit/qiskit-experiments/issues
-    """
+class CurveAnalysis(BaseCurveAnalysis):
+    """Base class for curve analyis."""


Can you also add a bit more to the class docstring to explain why we have this class? Currently it reads Base class for curve analyis. which is too brief given that we have another class called BaseCurveAnalysis.

releasenotes/notes/cleanup-curve-analysis-96d7ff706cae5b4e.yaml

qiskit_experiments/curve_analysis/__init__.py

chriseclectic

Initial review

qiskit_experiments/curve_analysis/curve_analysis.py

qiskit_experiments/curve_analysis/base_curve_analysis.py

qiskit_experiments/curve_analysis/curve_data.py

releasenotes/notes/cleanup-curve-analysis-96d7ff706cae5b4e.yaml

chriseclectic · 2022-04-25T15:50:38Z

qiskit_experiments/curve_analysis/__init__.py

+Curve Analysis Overview
+=======================
+
+The base class :class:`CurveAnalysis` implements the multi-objective optimization on


I wonder if this is starting to get too detailed for class level API docs and should instead be moved to its own separate User Guide on curve analysis?

Sounds good. Do we create user guide with this PR? I've already moved some long API docs (basically how code author can override method) to here and left :ref: link in the original place so I cannot simply remove this. Note that this is not a class level docs, this is module docs so I think this is probably okey for now.

Yeah this can be a separate PR, I think we should do a bit of planning on what would make some good mode detailed user guides

Co-authored-by: Daniel J. Egger <[email protected]>

Co-authored-by: Christopher J. Wood <[email protected]>

chriseclectic

Looks good, couple more documentation comments

qiskit_experiments/curve_analysis/base_curve_analysis.py

chriseclectic · 2022-04-25T21:23:12Z

qiskit_experiments/curve_analysis/curve_data.py

-    """Description of curve."""
+    """A dataclass to describe the definition of the curve.
+
+    Args:


I was curious what the "correct" way to comment a dataclass is and found this https://stackoverflow.com/a/51131941

They suggest preferring option (3) using Attributes: rather than using Args:. I didn't test myself, but I wonder if this will will override the empty attribute desciptions currently populated here. Checking the artifact on this PR the attribute doc stirngs are unchanged, but the Parameters: field is populated correctly from these args.

Can you try changing to attributes and checking what the built docs looks like and pick the one you think looks best?

Thanks for searching. I also prefer attribute style but I was not sure the best one. I just copied style from primitives in terra :)

releasenotes/notes/cleanup-curve-analysis-96d7ff706cae5b4e.yaml

LGTM now

eggerdj

This looks good to me. @chriseclectic as you have also commented on this PR I'll let you give the final approval.

releasenotes/notes/cleanup-curve-analysis-96d7ff706cae5b4e.yaml

Co-authored-by: Daniel J. Egger <[email protected]>

…wa1989/qiskit-experiments into feature/curve_analysis_baseclass

chriseclectic

LGTM

Refactor of CurveAnalysis and introduction of BaseCurveAnalysis class Co-authored-by: Daniel J. Egger <[email protected]> Co-authored-by: Christopher J. Wood <[email protected]>

nkanazawa1989 added the blocked Can't be worked on until another issue is resolved label Apr 6, 2022

nkanazawa1989 mentioned this pull request Apr 6, 2022

CurveAnalysis refactoring #737

Closed

7 tasks

nkanazawa1989 added this to the Release 0.3 milestone Apr 6, 2022

nkanazawa1989 force-pushed the feature/curve_analysis_baseclass branch 2 times, most recently from fb14230 to 5d3a33f Compare April 6, 2022 21:04

wip migration to curve analysis baseclass

3a4f605

nkanazawa1989 force-pushed the feature/curve_analysis_baseclass branch from 5d3a33f to 3a4f605 Compare April 6, 2022 21:08

eggerdj previously requested changes Apr 21, 2022

View reviewed changes

nkanazawa1989 and others added 3 commits April 22, 2022 04:39

Merge branch 'main' of github.com:Qiskit/qiskit-experiments into feat…

f9373d4

…ure/curve_analysis_baseclass

integration into main branch

220a421

review comment

9e7ad5b

Co-authored-by: Daniel J. Egger <[email protected]>

nkanazawa1989 removed the blocked Can't be worked on until another issue is resolved label Apr 21, 2022

nkanazawa1989 added 3 commits April 23, 2022 00:36

developer document in module doc

759eaea

update subclasses

3672723

update curvefit unittests

eb22e39

nkanazawa1989 changed the title ~~[WIP] CurveAnalysis base class~~ CurveAnalysis base class Apr 22, 2022

finalize

62d1609

- small docs fix - lint - reno

nkanazawa1989 force-pushed the feature/curve_analysis_baseclass branch from d3cde71 to 62d1609 Compare April 22, 2022 18:25

remove validation

79e8704

eggerdj reviewed Apr 25, 2022

View reviewed changes

chriseclectic reviewed Apr 25, 2022

View reviewed changes

review comments

3a86a25

Co-authored-by: Daniel J. Egger <[email protected]>

nkanazawa1989 force-pushed the feature/curve_analysis_baseclass branch from e7f83e9 to 3a86a25 Compare April 25, 2022 17:20

review comments

3d7a5e4

Co-authored-by: Christopher J. Wood <[email protected]>

nkanazawa1989 force-pushed the feature/curve_analysis_baseclass branch from 3d043d9 to 3d7a5e4 Compare April 25, 2022 18:46

readd method documentation and update reno

ef9e61b

chriseclectic reviewed Apr 25, 2022

View reviewed changes

docs and option name

d7e25a4

nkanazawa1989 force-pushed the feature/curve_analysis_baseclass branch from 38ceaa0 to d7e25a4 Compare April 25, 2022 22:09

chriseclectic reviewed Apr 25, 2022

View reviewed changes

releasenotes/notes/cleanup-curve-analysis-96d7ff706cae5b4e.yaml Outdated Show resolved Hide resolved

update reno with link

72d82e5

eggerdj reviewed Apr 26, 2022

View reviewed changes

releasenotes/notes/cleanup-curve-analysis-96d7ff706cae5b4e.yaml Outdated Show resolved Hide resolved

releasenotes/notes/cleanup-curve-analysis-96d7ff706cae5b4e.yaml Outdated Show resolved Hide resolved

nkanazawa1989 and others added 5 commits April 26, 2022 15:09

Update releasenotes/notes/cleanup-curve-analysis-96d7ff706cae5b4e.yaml

e714fc0

Co-authored-by: Daniel J. Egger <[email protected]>

Update releasenotes/notes/cleanup-curve-analysis-96d7ff706cae5b4e.yaml

6509b4a

Co-authored-by: Daniel J. Egger <[email protected]>

Merge branch 'main' into feature/curve_analysis_baseclass

9a92d1e

Merge branch 'feature/curve_analysis_baseclass' of github.com:nkanaza…

5f20a7b

…wa1989/qiskit-experiments into feature/curve_analysis_baseclass

test and lint fix

67c942b

nkanazawa1989 force-pushed the feature/curve_analysis_baseclass branch from 317c687 to 67c942b Compare April 26, 2022 07:08

nkanazawa1989 added 3 commits April 26, 2022 16:09

Merge branch 'main' into feature/curve_analysis_baseclass

0c9cb7c

minor typo fix

d9013ad

Merge branch 'feature/curve_analysis_baseclass' of github.com:nkanaza…

8c585b2

…wa1989/qiskit-experiments into feature/curve_analysis_baseclass

chriseclectic approved these changes Apr 26, 2022

View reviewed changes

chriseclectic merged commit 9fdc011 into qiskit-community:main Apr 26, 2022

nkanazawa1989 deleted the feature/curve_analysis_baseclass branch October 27, 2022 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CurveAnalysis base class #765

CurveAnalysis base class #765

nkanazawa1989 commented Apr 6, 2022 •

edited by chriseclectic

Loading

eggerdj left a comment

eggerdj Apr 21, 2022

nkanazawa1989 Apr 21, 2022

eggerdj Apr 25, 2022

nkanazawa1989 Apr 25, 2022 •

edited

Loading

eggerdj Apr 21, 2022

nkanazawa1989 Apr 21, 2022

eggerdj Apr 25, 2022

eggerdj Apr 25, 2022

nkanazawa1989 Apr 25, 2022

eggerdj left a comment •

edited

Loading

eggerdj Apr 25, 2022

chriseclectic left a comment

chriseclectic Apr 25, 2022

nkanazawa1989 Apr 25, 2022

chriseclectic Apr 26, 2022

chriseclectic left a comment

chriseclectic Apr 25, 2022

nkanazawa1989 Apr 25, 2022

eggerdj left a comment

chriseclectic left a comment

		A method to format curve data. By default this method takes y value average
		over the same x values and then sort the entire data by x values.

CurveAnalysis base class #765

CurveAnalysis base class #765

Conversation

nkanazawa1989 commented Apr 6, 2022 • edited by chriseclectic Loading

Summary

Details and comments

eggerdj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggerdj left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseclectic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseclectic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggerdj left a comment

Choose a reason for hiding this comment

chriseclectic left a comment

Choose a reason for hiding this comment

nkanazawa1989 commented Apr 6, 2022 •

edited by chriseclectic

Loading

nkanazawa1989 Apr 25, 2022 •

edited

Loading

eggerdj left a comment •

edited

Loading