Remove deprecated code for 0.7 #1452
Conversation
There was a problem hiding this comment.
Thanks @coruscating . Overall this looks good to me. Do you have any preference on change in dataframe option? It's scheduled in this RFC.
https://github.com/Qiskit/RFCs/blob/master/0007-experiment-dataframe.md#qiskit_experiments-v07
| result_data.append(overview) | ||
| # Store fit status overview entry regardless of success. | ||
| # This is sometime useful when debugging the fitting code. | ||
| overview = AnalysisResultData( |
There was a problem hiding this comment.
Why do we keep this object? Now it's stored in the artifact right? Is this due to the index change of the analysis results? I would turn default option value into None and issue the deprecation warning unless user doesn't explicitly set this to False (maybe too annoying though). With this change we'll lose the knob to trigger deprecation warning in future when we remove this object, i.e. we cannot raise the warning when Python list [0] is called for the analysis results.
There was a problem hiding this comment.
We had some discussions last release about how to deprecate/remove this entry (#1342 (comment)). I think since we've had one release cycle with artifacts it's ok to remove this directly. What do you think @wshanks?
There was a problem hiding this comment.
The reason we kept this before was because many tests used integer arguments to analysis_results() and I worried that that likely reflected a common pattern in user code as well. So removing the first result would change the integer indices of all other results.
I don't think it is that big of a burden to leave the overview code in for now. I think I would leave it in along with return_fit_parameters until either passing an integer or slice or passing dataframe=False to analysis_results() is removed. What is the timeline for those removals?
There was a problem hiding this comment.
Maybe deprecation for
- When search index is int type or None, and
- When the result includes overview entry
it the reasonable approach?
There was a problem hiding this comment.
@nkanazawa1989 we are already issuing deprecating warnings for int/slice search index and retrieving the overview entry: https://github.com/Qiskit-Extensions/qiskit-experiments/blob/fe4ccff973dae85fdff3416b253880051b71e6c3/qiskit_experiments/framework/experiment_data.py#L1569-L1582
Do you want to implement this differently?
There was a problem hiding this comment.
Yeah, I'm fine with leaving it as is until we're ready to make the full change since artifacts (and figures) are also currently accessible via the integer key. I think it's awkward for the behavior to be different across these interfaces. What do you think @nkanazawa1989? Maybe we just deprecate retrieving artifacts by index in this release?
There was a problem hiding this comment.
Sounds like a plan. Let's deprecate it as well in this release and remove all of them in 0.8.
There was a problem hiding this comment.
@coruscating I see you added the deprecation. Can we also put back the return_fit_parameters option until we remove returning the overview completely?
There was a problem hiding this comment.
Done, this is ready for another look.
There was a problem hiding this comment.
The part touched on here looks good now. The rest of the PR looks good as well but I didn't review very closely and will leave it to @nkanazawa1989.
| extra (Dict[str, Any]): A dictionary that is appended to all database entries | ||
| as extra information. | ||
| """ | ||
| options = super()._default_options() | ||
| options.update_options( | ||
| plotter=CurvePlotter(MplDrawer()), | ||
| plot=True, | ||
| return_fit_parameters=False, |
There was a problem hiding this comment.
| return_fit_parameters=False, | |
| return_fit_parameters=True, |
I think this has been True because options value comes from the base class. For example Ham tomo experiment has returned the fit parameter (in the unittest we just don't search by index)
There was a problem hiding this comment.
Hmm I ran ham tomo on current code but didn't see the fit parameter:
You think we should change this behavior? I don't remember why return_fit_parameters was set to false for composite curve analysis.
There was a problem hiding this comment.
I realized we unintentionally introduced breaking API change in the previous release.
https://github.com/Qiskit-Extensions/qiskit-experiments/blob/bc0b89e0eeedfabab7de2d5b6c5472e26bdbfb5a/qiskit_experiments/curve_analysis/composite_curve_analysis.py#L373-L375
We should have duplicated this combined_summary in the analysis results, but we only added this to artifact of the composite curve analysis. So the data has been moved to artifact without any warning. So far we didn't see any report in our issue, and I think this bug is not enough serious to motivate us to make a patch release for 0.6. And we don't need to redefine return_fit_parameters in the composite curve analysis options. What do you think?
There was a problem hiding this comment.
Ah, thanks for finding the source of the problem. How about I just keep the code the same and add a note in the Known Issues section of the release notes?
Removes code scheduled for removal in 0.7. For analysis results,
dataframeis still set toFalseby default asTrueis a breaking change.