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

Remove pytket-quantinuum dependency. #107

Merged
merged 7 commits into from
Jan 26, 2024
Merged

Remove pytket-quantinuum dependency. #107

merged 7 commits into from
Jan 26, 2024

Conversation

cqc-alec
Copy link
Collaborator

@cqc-alec cqc-alec commented Jan 25, 2024

And remove the tket_optimization_level argument to pytket_to_phir(). Add a gateset field to the Machine class and populate the QtmMachine instances with the current supported gatesets. Apply a standard rebase pass after decomposing boxes (i.e. optimization level 0).

Create a dataclass for machine timing data to get around ruff's (quite reasonable) complaint that there were too many arguments to Machine.__init__().

@cqc-alec cqc-alec requested a review from qartik January 25, 2024 13:40
@cqc-alec cqc-alec marked this pull request as ready for review January 25, 2024 13:40
Copy link
Member

@qartik qartik left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking great. Great idea to abstract timings into its own data class.

README update is the only needed change, rest are suggestions.

tests/test_placement.py Outdated Show resolved Hide resolved
pytket/phir/qtm_machine.py Outdated Show resolved Hide resolved
pytket/phir/cli.py Show resolved Hide resolved
@cqc-alec cqc-alec requested a review from qartik January 25, 2024 16:52
tests/e2e_test.py Outdated Show resolved Hide resolved
qartik
qartik previously approved these changes Jan 25, 2024
Copy link
Member

@qartik qartik left a comment

Choose a reason for hiding this comment

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

LGTM. I'd wait for @neal-erickson's review before merging this. Also, left a minor comment.

neal-erickson
neal-erickson previously approved these changes Jan 25, 2024
Copy link
Contributor

@neal-erickson neal-erickson left a comment

Choose a reason for hiding this comment

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

One question about approach to the "no machine" handling but otherwise looks good to me.

api_handler=qapi_offline, # type: ignore [arg-type]
machine = QTM_MACHINES_MAP.get(qtm_machine)
gateset = (
{OpType.Rz, OpType.PhasedX, OpType.ZZPhase}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is roughly equivalent to the Quantinuum gateset, do we want to rebase to that in the case of no machine - seems like it might reduce the flexibility of allowing this to be run without any Quantinuum specificity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the code to use QTM_DEFAULT_GATESET explicitly. I don't think this reduces flexibility, as one can specify a machine with another gateset if desired.

@cqc-alec cqc-alec dismissed stale reviews from neal-erickson and qartik via 08b6934 January 26, 2024 08:43
@qartik qartik merged commit 711192a into main Jan 26, 2024
6 checks passed
@qartik qartik deleted the rmquum branch January 26, 2024 20:17
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