Integration with result database#115
Conversation
Co-authored-by: Jessie Yu <jessieyu@us.ibm.com>
| except AnalysisError as ex: | ||
| analysis_results = [AnalysisResult(success=False, error_message=ex)] | ||
| figures = None | ||
| analysis_results, figures = self._run_analysis(experiment_data, **analysis_options) |
There was a problem hiding this comment.
Is it OK to remove the try-catch here? The difficulty is in populating the parameters of AnalysisResultV1 in case of exception. @jyu00 @nkanazawa1989 @chriseclectic
There was a problem hiding this comment.
I think I removed the try/catch, and my reasoning was that we shouldn't be saving a failed analysis result.
| analysis_result["raw_data"] = raw_data_dict | ||
| result_data["raw_data"] = raw_data_dict | ||
|
|
||
| analysis_result = AnalysisResultV1( |
There was a problem hiding this comment.
Should this line be inside or outside "finally" ?
| result_data=result_data, | ||
| result_type=result_data["analysis_type"], | ||
| device_components=[ | ||
| Qubit(qubit) for qubit in experiment_data.data(0)["metadata"]["qubits"] |
There was a problem hiding this comment.
Is there a new way to retrieve the qubit, through experiment metadata?
There was a problem hiding this comment.
I don't think qubit is currently added to experiment metadata, but it'd be a good thing to do. It'd certainly make this line easier.
|
@jyu00 What happened to |
|
@jyu00 While debugging failing tests, I see that for But the post-processing callback seems not be called; a print at the beginning of In the test, we call
|
|
@yaelbh In my other PR I added a new
the BUT I realized the error in my logic - it still wouldn't have helped if job2 finishes before job1. So I removed it and just added a warning if I also added code to log a warning if
|
|
@mtreinish @jyu00 |
|
When I save a T2 Ramsey experiment, the analysis results are in the database but the figure is not. Doesn't necessarily need to be fixed in this PR, but just noting the problem. |
|
I'm curious about the result themselves for T2Ramsey, because they are non-orthodox, and I expected that they'd need to change in order to display on IQX. There is no field name "value", but two fields named "t2ramsey_value" and "frequency_value". Can you paste what you see in IQX? |
|
@chriseclectic 09538f7 has code that allows all objects to be "serialized" (it just uses the class name). This should at least allow tomography to be saved in results DB. Perhaps we can do the |
| result_data["raw_data"] = raw_data_dict | ||
|
|
||
| return [analysis_result], figures | ||
| return [CurveAnalysisResultData(result_data)], figures |
There was a problem hiding this comment.
Why CurveAnalysisResultData is generated again with nested CurveAnalysisResultData?
There was a problem hiding this comment.
Sorry, I thought it was a dictionary, I'll fix. Also need to fix in T1 and T2Ramsey, where it should be CurveAnalysisResultData instead of AnalysisResultData.
chriseclectic
left a comment
There was a problem hiding this comment.
Looks pretty close. I think we can still remove the custom CurveAnalysisResult and CurveAnalysisResultData
|
|
||
|
|
||
| class CurveAnalysisResult(AnalysisResult): | ||
| class CurveAnalysisResultData(AnalysisResultData): |
There was a problem hiding this comment.
Can this class be deleted now and just use AnalysisResultData?
There was a problem hiding this comment.
@nkanazawa1989 Are you good with this or do you want to keep CurveAnalysisResultData as type hint?
There was a problem hiding this comment.
Yes, I'm fine with removing this specific class if the hint will become other than Dict
| bounds: Optional[Union[Dict[str, Tuple[float, float]], Tuple[np.ndarray, np.ndarray]]] = None, | ||
| **kwargs, | ||
| ) -> CurveAnalysisResult: | ||
| ) -> CurveAnalysisResultData: |
There was a problem hiding this comment.
| ) -> CurveAnalysisResultData: | |
| ) -> AnalysisResultData: |
| self, experiment_data: ExperimentData, **options | ||
| ) -> Tuple[List[AnalysisResult], List["matplotlib.figure.Figure"]]: | ||
| ) -> Tuple[ | ||
| Union["AnalysisResultData", List["AnalysisResultData"]], List["matplotlib.figure.Figure"] |
There was a problem hiding this comment.
| Union["AnalysisResultData", List["AnalysisResultData"]], List["matplotlib.figure.Figure"] | |
| Tuple[List["AnalysisResultData"], List["matplotlib.figure.Figure"]] |
| plot=True, | ||
| ax=None, | ||
| ) -> Tuple[List[AnalysisResult], List["matplotlib.figure.Figure"]]: | ||
| ) -> Tuple[List[AnalysisResultData], Optional[List["matplotlib.figure.Figure"]]]: |
There was a problem hiding this comment.
Is Optional correct type hint when it returns either List or None. Maybe we should change to
| ) -> Tuple[List[AnalysisResultData], Optional[List["matplotlib.figure.Figure"]]]: | |
| ) -> Tuple[List[AnalysisResultData], List["matplotlib.figure.Figure"]]: |
and return [] instead of None if there is no mpl.
|
|
||
| analysis_result["fit"]["circuit_unit"] = unit | ||
| result_data = { | ||
| "t2ramsey_value": fit_result["popt"][1], |
There was a problem hiding this comment.
For a follow up PR. But I think this needs to be changed into two analysis results if you want to show both T1 value and frequency value in DB.
chriseclectic
left a comment
There was a problem hiding this comment.
Lets worry about my previous comment on repr of (Curve)AnalysisResultData objects in a follow up PR.
* move from terra * run black * convert service exception to log * fix lint * Integration with result database Co-authored-by: Jessie Yu <jessieyu@us.ibm.com> * some brief fixes * log post processing failure * remove data index * fix t1 t2 tests * fix test can package typo * fix package typo * fixed errors in conlicts resolve * adjusted a test to the new classes * made test_spectroscopy_end2end_classified pass * adjusted all spectro tests * test_curve_fit passes * adjusted rabi * black * lint * fixed test_source * fix doc build * fix figure thread error * run black * remove ResultDict * remove experiment_class and result_class * fix lint * fixes * adjusted tes_update_library * lint * black * doc update Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * rename classes * review comments * fix lint * black * black * catch failed analysis + black * omit prefix computer_ from quality * removed debug print * fix docs build * added meas_level to mock iq backend header * black * removed the call to get_memory * adjusted test_rb_analysis * fix analysis result __str__ * add callable to encoder * fix lint * fix lint again * log all job failure * lint * fix test * return DbAnalysisResultV1 for t1 t2 * review comments * Add `load` method to `DbExperimentDataV1` and `DbAnalysisResultV1` This method parses the dicts currently returned by the IBMQ service to allow loading saved experiment data and analysis results. * Revert "Merge pull request qiskit-community#2 from chriseclectic/deserialize" This reverts commit e150dad, reversing changes made to 0d6e3c9. * updated tomography qubits * Add `load` method to `DbExperimentDataV1` and `DbAnalysisResultV1` This method parses the dicts currently returned by the IBMQ service to allow loading saved experiment data and analysis results. * Rename `save` to `save_metadata`, `save_all` to `save` * changed analysis result classes * black * lint * fixes related to AnalysisResultData * allow all objects in encoder * call save if auto save set * fix tests * fixed test_fine_amplitude * lint * black * adjust to recent changes in main * fixes related to AnalysisResultData Co-authored-by: jessieyu <jessieyu@us.ibm.com> Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> Co-authored-by: Christopher Wood <cjwood@us.ibm.com>

Summary
This PR is based on #113, and is a continuation of #66
Details and comments