-
Notifications
You must be signed in to change notification settings - Fork 1
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
use pytket-qir for qir generation #24
Conversation
_metadata.py
Outdated
@@ -1,2 +1,2 @@ | |||
__extension_version__ = "0.3.0" | |||
__extension_version__ = "0.4.0rc0" |
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.
Do we need an rc for this?
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.
------------------------ | ||
|
||
* Update minimum pytket version to 1.37.0. | ||
* Update minimum pytket-qir version to 0.19.0. |
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.
This seems incomplete, are there no other changes we should list?
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 have added a few more details, see remove qiskit, update changelog and version
3a89c6e
setup.py
Outdated
@@ -56,9 +56,10 @@ | |||
include_package_data=True, | |||
install_requires=[ | |||
"azure-quantum >= 2.2.0", | |||
"pytket >= 1.34.0", | |||
"pytket >= 1.37.0", | |||
"pytket-qiskit >= 0.58.0", | |||
"qiskit-qir >= 0.5.0", |
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.
If we're not using it we can we remove this dependency (but as commented elsewhere I think we should continue to use it for other devices rather than stop supporting them).
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 have removed them, see remove qiskit, update changelog and version
3a89c6e
cut_pytket_register=False, | ||
profile=QIRProfile.AZUREBASE, | ||
) | ||
raise ValueError("ionq devices currently not supported") |
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.
Rather than drop support for ionq can we continue to use the old method (using qiskit-qir) in this case?
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 had removed this, because this had issues with a lot of gates and it is unclear what is supported / should be supported. I have added this now back in with the same support as we had before. This is probably fine, as this is all not a state to use in production.
@@ -37,7 +45,9 @@ def test_ionq_simulator(azure_backend: AzureBackend) -> None: | |||
counts = r.get_counts() | |||
assert counts == Counter({(0, 0): 5, (1, 1): 5}) | |||
else: | |||
warn("ionq.simulator unavailable or queue time >= 60s: not submitting") |
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.
Why change this? I think it should be a warning not an error.
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 found it really tricky to debug this with only getting warnings. If you want to, I can move back to the warnings, but I think as long as we are in the testing stage, it would be better to keep this as an error.
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.
OK
@@ -52,19 +62,148 @@ def test_quantinuum_sim_h11e(azure_backend: AzureBackend) -> None: | |||
counts = r.get_counts() | |||
assert sum(counts.values()) == 1000 | |||
else: | |||
warn("quantinuum.sim.h1-1sc unavailable or queue time >= 60s: not submitting") |
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.
Same comment (and likewise in other tests below).
tests/backend_test.py
Outdated
counts = r.get_counts() | ||
assert all(x0 == x1 for x0, x1 in counts) | ||
assert all(x[0] == x[1] for x in counts) | ||
assert any(x[0] == 1 for x in counts) |
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.
This is likely but not guaranteed.
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 thought this is a valid assumption for 1000 shots. If you want me to remove this, I will remove it.
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 have added a comment to make this clear.
@@ -37,7 +45,9 @@ def test_ionq_simulator(azure_backend: AzureBackend) -> None: | |||
counts = r.get_counts() | |||
assert counts == Counter({(0, 0): 5, (1, 1): 5}) | |||
else: | |||
warn("ionq.simulator unavailable or queue time >= 60s: not submitting") |
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.
OK
Co-authored-by: Alec Edgington <[email protected]>
Description
Please summarise the changes.
Related issues
Please mention any github issues addressed by this PR.
Checklist