Skip to content
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

[SDS-307] Now able to get calibration data from result #103

Merged
merged 3 commits into from
Jun 14, 2021

Conversation

QFer
Copy link
Contributor

@QFer QFer commented Jun 3, 2021

  • Added method to get calibration info to api
  • Added unit tests

@QFer QFer requested a review from eendebakpt June 9, 2021 09:50
@@ -676,6 +676,32 @@ def get_measurement_register_from_result(self, result_id: int) -> List[Any]:
raise ApiError(f'Measurement register for result with id {result_id} does not exist!') from err_msg
return measurement_register

def get_calibration_from_result(self, result_id: int) -> Optional[Any]:
""" Gets the calibration information of the executed cQASM code, given the result_id.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
""" Gets the calibration information of the executed cQASM code, given the result_id.
""" Gets the calibration information of the executed code, given the result_id.

We also submit qiskit code (right that that is converted to cqasm internally, but that might change)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to executed job. Also changed some other instances of cQASM (where it is not needed to be called cQASM explicitly)

result = self.get_result(result_id)
calibration_url = result.get('calibration')
if calibration_url is not None:
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this code works, but is the parsing of the url needed? One can just do

result = requests.get(c, auth=authentication)
calibration = result.json()

(the authentication and .json can be wrapped in something off course)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you can directly use request package to send a get for this url,, but I think it is better to use the already authenticated coreapi consistently for all communication with the backend api as it is now done in SDK.

peendebak
peendebak previously approved these changes Jun 11, 2021
Copy link
Contributor

@peendebak peendebak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality here is good. There is still the issue of documenting the (possible) confusion between the job_id and project_id, but that should be done in another PR.

@QFer
Copy link
Contributor Author

QFer commented Jun 11, 2021

Had to force push after rebase

@coveralls
Copy link
Collaborator

Coverage Status

Coverage remained the same at 100.0% when pulling 5228be3 on fix/sds-307/calibration-info into d99bada on dev.

@QFer QFer merged commit 81afdb9 into dev Jun 14, 2021
@QFer QFer deleted the fix/sds-307/calibration-info branch November 18, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants