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

check if default two qubit gate is supported by the device #472

Merged
merged 9 commits into from
Jul 16, 2024

Conversation

cqc-melf
Copy link
Collaborator

Description

Solve #457

Checklist

  • I have performed a self-review of my code.
  • I have commented hard-to-understand parts of my code.
  • I have made corresponding changes to the public API documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the changelog with any user-facing changes.

@cqc-melf cqc-melf requested a review from yao-cqc as a code owner July 11, 2024 13:04
@cqc-melf cqc-melf marked this pull request as draft July 11, 2024 13:04
@cqc-melf cqc-melf force-pushed the melf/fix-unsupported-2q-gate branch from f9d9f13 to 5f918e8 Compare July 11, 2024 15:48
@cqc-melf cqc-melf requested a review from cqc-alec July 11, 2024 15:53
@cqc-melf cqc-melf marked this pull request as ready for review July 11, 2024 15:53
@yao-cqc
Copy link
Contributor

yao-cqc commented Jul 11, 2024

Can we instead check this in the QuantinuumBackend constructor and use the same error message in set_compilation_config_target_2qb_gate?

@cqc-melf
Copy link
Collaborator Author

set_compilation_config_target_2qb_gate

set_compilation_config_target_2qb_gate already has a check, so I think that should be fine?

Unfortunately I can't use _gate_set in the constructor because that calls the API and breaks quiet a few things for backends constructed with the offlineapi.

Happy to find a workaround for that if you think that is important.

@cqc-alec
Copy link
Collaborator

It does feel wrong to set this config value inside rebase_pass(). Can we change the _default_2q_gate() function instead?

@cqc-alec
Copy link
Collaborator

By the way, deadhead and brathead both now accept the ZZPhase gate. So maybe we don't need this anymore, as ZZPhase is available on all devices?

@yao-cqc
Copy link
Contributor

yao-cqc commented Jul 12, 2024

set_compilation_config_target_2qb_gate

set_compilation_config_target_2qb_gate already has a check, so I think that should be fine?

Unfortunately I can't use _gate_set in the constructor because that calls the API and breaks quiet a few things for backends constructed with the offlineapi.

Happy to find a workaround for that if you think that is important.

I think self._gate_set won't make any API call if the backend is constructed with the offline API. Maybe I misunderstood the problem?

@cqc-melf
Copy link
Collaborator Author

By the way, deadhead and brathead both now accept the ZZPhase gate. So maybe we don't need this anymore, as ZZPhase is available on all devices?

I think we discussed this in the last sprint planing that we still want to have this check additionally to make this future proof.

If you think this is not required, then we can close this.

@cqc-melf
Copy link
Collaborator Author

set_compilation_config_target_2qb_gate

set_compilation_config_target_2qb_gate already has a check, so I think that should be fine?
Unfortunately I can't use _gate_set in the constructor because that calls the API and breaks quiet a few things for backends constructed with the offlineapi.
Happy to find a workaround for that if you think that is important.

I think self._gate_set won't make any API call if the backend is constructed with the offline API. Maybe I misunderstood the problem?

I think I was wrong with the problem detection, the actual issue is that we have tests are not expected to make API calls that would make this calls with the changes in the constructor.

I have now gone the way and updated all the affected testcases.

@cqc-melf
Copy link
Collaborator Author

The failed tests on QA are timeouts, so should be fine to merge without them.

Copy link
Collaborator

@cqc-alec cqc-alec left a comment

Choose a reason for hiding this comment

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

I'm just a little worried that requiring login when constructing QuantinuumBackend may break some workflows; perhaps we should check this with nexus?

@cqc-melf
Copy link
Collaborator Author

I'm just a little worried that requiring login when constructing QuantinuumBackend may break some workflows; perhaps we should check this with nexus?

I checked with them, this is not a concern.

@cqc-melf
Copy link
Collaborator Author

I will take a look at the failed notebook and update that. Will ask for a review after that is updated.

@cqc-alec
Copy link
Collaborator

I will take a look at the failed notebook and update that. Will ask for a review after that is updated.

This is probably #474 . If so it needs an update from the US side.

cqc-alec
cqc-alec previously approved these changes Jul 16, 2024
@cqc-melf cqc-melf requested a review from cqc-alec July 16, 2024 09:51
@cqc-melf
Copy link
Collaborator Author

I will take a look at the failed notebook and update that. Will ask for a review after that is updated.

This is probably #474 . If so it needs an update from the US side.

Yes, looks like it.

Made one push for testing, can you approve this gain? @cqc-alec

@@ -1038,7 +1038,7 @@ def process_circuits(
"max_classical_register_width", 32
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to change the default we should change it here too, but I'm not sure if we want to do this.

@cqc-melf cqc-melf merged commit e27e08b into main Jul 16, 2024
10 of 14 checks passed
@cqc-melf cqc-melf deleted the melf/fix-unsupported-2q-gate branch July 16, 2024 09:57
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.

3 participants