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

Recursively expand defcals #1270

Merged
merged 15 commits into from
Nov 19, 2020
Merged

Recursively expand defcals #1270

merged 15 commits into from
Nov 19, 2020

Conversation

notmgsk
Copy link
Contributor

@notmgsk notmgsk commented Nov 4, 2020

This PR does two things:

  • Recursively expand calibrations, meaning that if a calibration expansion introduces new instructions that have an associated calibration then those instructions will also be expanded (and so on).

  • Extends the grammar to permit "formal" arguments to Gate outside of DEFCIRCUIT. This is required because the above change (calibration expansion) is motivated by calibration bodies possibly including gates, and since a calibration can be specified on a qubit variable (e.g. q rather than 0) then the body of the calibration needs to support qubit variables. Previously this was only allowed within a circuit definition, but to make life simpler, it is now possible anywhere a gate is permitted.

@notmgsk notmgsk requested a review from kalzoo November 4, 2020 21:58
@notmgsk notmgsk requested a review from a team as a code owner November 4, 2020 21:58
@notmgsk notmgsk requested a review from axdhill November 4, 2020 21:58
@notmgsk notmgsk changed the base branch from master to quilt-demo November 4, 2020 21:58
Copy link
Contributor

@kalzoo kalzoo left a comment

Choose a reason for hiding this comment

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

Thanks! As per our chat, just a couple tweaks:

  1. Regenerate with ANTLR 4.8
  2. account for and guard on expansion recursion depth. Use a module constant for maximum depth probably

@notmgsk
Copy link
Contributor Author

notmgsk commented Nov 6, 2020

Thanks! As per our chat, just a couple tweaks:

1. Regenerate with ANTLR 4.8

Done.

2. account for and guard on expansion recursion depth. Use a module constant for maximum depth probably

Also done, though by a different method. Instead of testing that calibration isn't going too deep, I test that there's no cycle in the calibrations which I think is the more likely issue (i.e. user erroneously supplies a bad DEFCAL). If the user supplies enough (acyclic) calibrations that cause a memory error or w/e, then that's on them.

@notmgsk
Copy link
Contributor Author

notmgsk commented Nov 6, 2020

There's a strange build error happening on travis-ci. Looks like test_invalid_protocol is hanging. Not sure why (travis-ci doesn't interrupt it and provide a backtrace), but it doesn't fail locally.

Copy link
Contributor

@kalzoo kalzoo left a comment

Choose a reason for hiding this comment

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

Should the grammar rely on the submodule now?

pyquil/quil.py Outdated Show resolved Hide resolved
pyquil/quil.py Outdated
expanded_instructions = expand_calibration(match)
for expanded_instruction in expanded_instructions:
if expanded_instruction in seen_instructions:
raise RuntimeError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a more-expressive error class to use? Is it worth adding a CalibrationError for this and other errors in calibration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have defined that exception class. There already exists a CalibrationDoesntMatch class, which now has CalibrationError as its parent. Should we define a new child class CyclicalCalibrationError? or is that going too far?

pyquil/tests/test_quilt.py Outdated Show resolved Hide resolved
assert calibrated == Program("RY(pi) 0").instructions


def test_program_calibrate_cyclic_error():
Copy link
Contributor

@kalzoo kalzoo Nov 17, 2020

Choose a reason for hiding this comment

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

would prefer separate or parametrized test cases for these independent programs, i.e.

@pytest.mark.parametrize('program_text', ('DEFCAL RZ(%theta) q:\n\tRZ(%theta) q', ...))
def test_program_calibrate_cyclic_error(program_text):
    ...

@notmgsk
Copy link
Contributor Author

notmgsk commented Nov 18, 2020

Restored the grammar symlink. Must've gotten lost in a rebase.

@notmgsk notmgsk merged commit ee7417d into quilt-demo Nov 19, 2020
notmgsk added a commit that referenced this pull request Nov 24, 2020
- recursively expand defcals
- allow formal qubits in top-level gates
@dbanty dbanty deleted the recursively-expand-defcals branch February 14, 2022 17:04
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.

2 participants