-
Notifications
You must be signed in to change notification settings - Fork 131
Don't update verified automatically and forward chisq to experiment service
#200
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
Conversation
|
Necessary (but insufficient) to fix #194 (the |
| # Load data from the service | ||
| return cls._from_service_data(service.analysis_result(result_id)) | ||
| instance = cls._from_service_data(service.analysis_result(result_id)) | ||
| instance._created_in_db = True |
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.
The DbExperimentDataV1.load method probably also needs to have this attribute set on the object it returns.
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.
Good point.
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.
Fixed in c6d0a9f.
| if "success" not in res: | ||
| res["success"] = True | ||
| analysis_result = DbAnalysisResultV1(result_data=res, **analysis_result_parameters) | ||
| if "chisq" in res: |
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.
The existing experiments will need to be updated to use this label, at the moment they all populate "reduced_chisq"
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.
We could keep that if you prefer.
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 don't have a strong preference, either way is fine with me
|
What you did for |
|
@yaelbh if you look at the service endpoint (https://github.ibm.com/IBM-Q-Software/sw-ibmq-results/blob/staging/docs/api-ref.md#post-analysis_results---create-analysis-result) you can see that expects |
chriseclectic
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.
We still need to decide whether to do chisq or reduced_chisq for experiments, but I think this is good to merge for the verified and load fix.
…nt service (qiskit-community#200) Co-authored-by: Christopher Wood <cjwood@us.ibm.com>
Summary
The
verifiedfield on analysis results was being set wheneverqualitywas set. This incorrectly tied together the meaning of these two fields (verifiedreferences human review).χ² was missing a means to forward to the experiment service. I have added this as a property to
DbAnalysisResultsV1.Details and comments
During testing I discovered that I could not call
save()on aDbAnalysisResultV1object that I hadloaded from the service. Setting the_created_in_dbfield was sufficient to fix this.