-
Notifications
You must be signed in to change notification settings - Fork 131
Curve Analysis class #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Curve Analysis class #46
Conversation
yaelbh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks very good. My comments are mostly questions/hesitations and typos.
| Returns: | ||
| New AnalysisResult instance containing the result of post analysis. | ||
| """ | ||
| return analysis_result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, not sure whether to return a dictionary or an AnalysisResultV1 object
Co-authored-by: Yael Ben-Haim <yaelbh@il.ibm.com>
eggerdj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some ideas on how to better deal with the data processing.
Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com>
Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com>
gadial
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the PR is really excellent and will save some of the current complexity in RB; however, I'm not sure if it'll be possible to use RB with it without some further additions (in particular the handling of p0).
- add default data processor - add data processor calibraiton - integrate run_fit and series fit - add data pre-processing (i.e. RB needs to take mean) - add fitter setup for initial guess and other options
…kit-experiments into feature/curve_analysis
…ure/curve_analysis
eggerdj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the training logic. I still have a few questions and comments related to it.
| return AnalysisResult(result) | ||
|
|
||
|
|
||
| def level2_probability(data: Dict[str, Any], outcome: Optional[str] = None) -> Tuple[float, float]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
level2_probability can be removed, see my comment above.
…kit-experiments into feature/curve_analysis
eggerdj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good. Still a few comments and questions.
Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com>
Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com>
Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com>
Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com>
Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com>
- add y normalization
Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com>
Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com>
eggerdj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* wip curve fit analysis * wip curve fit analysis * curve_fit complete * - unittest - bug fix - lint * black * removed redundant code * fix unittest * fix docstring * fix docstring * wording fix Co-authored-by: Yael Ben-Haim <yaelbh@il.ibm.com> * Update qiskit_experiments/analysis/curve_analysis.py Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> * Update qiskit_experiments/analysis/curve_analysis.py Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> * Feedback from eggerdj - add default data processor - add data processor calibraiton - integrate run_fit and series fit - add data pre-processing (i.e. RB needs to take mean) - add fitter setup for initial guess and other options * readd 52c99ea and 0420d1d * add fit functions * lint * review by eggerdj r2 - change dp calibration logic - add gaussian function * add data processor keys in metadata add data processor keys in metadata * conform to qiskit-community#41 * feedback from chris1 - more general docstring - add default parameter to fit funcs * feedback from chris2 - parameter dict handling * feedback from chris3 - import path * lint * add fit option validation * add unittest and integration test * black & lint * simplify the analysis class * misc * add default figure generation * lint * add default value * fix docstring typo * remove outcome from default processing options * black and lint * add processor training check * fix pre processing routine * update rb analysis as an example - add num_qubits to curve analysis - add label generation to curve analysis * post process error handling when fit failed * add axis label * update axis formatting * more integration with new options * update irb * move class attributes to options * review comment from Chris * fix bug causes list indices must be ... * Analysis result formatting * update error docstring * lint * bug fix - reorder args of protected methods (series comes before xvals) - make analysis result list - relevant test fix - add filter kwargs to interleaved analysis * lint * fix spect analysis * fix composite analysis to use instance's analysis option * adjust curve figure appearance * black * update data and option handling * - private -> protected member - param list generation and validation in new method - all nan sigma handling - zero sigma handling in curve fitter - add data extraction utils - allow bounds = None - * update spectroscopy * black & lint * rerun rb notebook * Update qiskit_experiments/analysis/curve_analysis.py Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> * Update qiskit_experiments/analysis/curve_analysis.py Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> * Update qiskit_experiments/analysis/curve_analysis.py Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> * Update qiskit_experiments/analysis/curve_analysis.py Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> * Update qiskit_experiments/analysis/curve_analysis.py Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> * - more docstring - add y normalization * black * add TODO comment * add analysis class information to result data * fix unittest * update spect analysis docstring * Update qiskit_experiments/analysis/curve_analysis.py Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> * rb analysis class docstring * format docstring * Update qiskit_experiments/analysis/curve_analysis.py Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> Co-authored-by: Yael Ben-Haim <yaelbh@il.ibm.com> Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>
Summary
This PR adds
CurveAnalysisclass that provides basic functionalities to extract curve data and perform proper error handling for subclasses.Details and comments
Based on this base class, we can easily implement several fitting subclasses. Data extraction and error handling is nicely performed behind the scene. This class overrides
BaseAnalysis._run_analysisand provides several function-wise private methods that subclasses can override. For example_run_fitting,_create_figure,_data_processing, and_post_processing. Internal data representationCurveEntry(named tuple) is introduced to manage curve data.To define new analysis, basically you can override base class attributes.
For example, T1 experiment fit may be written as
and IRB fit may be written as
As you can see subclasses don't need to write their own code. This will give us two benefits:
Hint for review
Details are written in the CurveAnalysis class docstring.
Actual usages are shown in unittest.
Flow of fit operation
_run_analysisis called_run_analysiscalls_extract_curves._extract_curvescalls_data_processing(so that subclass can override the logic)._extract_curvesgeneratesCurveEntrys and return them._run_analysiscalls_run_fitting(so that subclass can override the logic, i.e. initial guess/weights/bounds...)_run_fittingcalls_series_curve_fit_series_curve_fitperforms a linearized multi-objective non-linear least squares fit and returnsAnalysisResults._run_analysiscalls_post_processingfollowed by_create_figure._run_analysisreturns analysis results and figures