-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Delete SelectionRegisters and replace uses of Registers with Tuple[Register, ...]
#6278
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
Delete SelectionRegisters and replace uses of Registers with Tuple[Register, ...]
#6278
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #6278 +/- ##
=======================================
Coverage 97.88% 97.88%
=======================================
Files 1106 1106
Lines 95710 95699 -11
=======================================
- Hits 93683 93674 -9
+ Misses 2027 2025 -2
☔ View full report in Codecov by Sentry. |
mpharrigan
left a comment
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.
Looks like mostly rote changes, which lgtm
What do you think about documenting how to use ravel and unravel for multiple selection registers? Presumably if you didn't know about it originally others may have the same problem! Where would be the most discoverable place to put this? Maybe a paragraph in the SelectionRegister docstring
| return f'cirq_ft.Register(name="{self.name}", shape={self.shape})' | ||
|
|
||
|
|
||
| def total_bits(registers: Iterable[Register]) -> int: |
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 know none of these had docstrings, but it might be a good time to add a quick one-line docstring to each
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.
added one line docstrings
| regs = [cirq_ft.SelectionRegister('x', n, N), cirq_ft.SelectionRegister('y', m, M)] | ||
| for x in range(regs[0].iteration_length): | ||
| for y in range(regs[1].iteration_length): | ||
| assert np.ravel_multi_index((x, y), (N, M)) == x * M + y |
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.
good ol' ravel
| target_regs = {reg.name: quregs[reg.name] for reg in self.target_registers} | ||
| extra_regs = {reg.name: quregs[reg.name] for reg in self.extra_registers} |
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.
👍
|
@mpharrigan Addressed your comments. Added a detailed docstring to |
This PR brings the
cirq_ft.Registerscloser toqualtran.Signature. It's part of a larger effort to consolidatecirq-ftandqualtrandata types.