Skip to content

Conversation

@tanujkhattar
Copy link
Collaborator

This PR brings the cirq_ft.Registers closer to qualtran.Signature. It's part of a larger effort to consolidate cirq-ft and qualtran data types.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CirqBot CirqBot added the size: L 250< lines changed <1000 label Sep 6, 2023
@tanujkhattar tanujkhattar added BREAKING CHANGE For pull requests that are important to mention in release notes. area/cirq-ft Issues related to the Cirq-FT sub-package and removed size: L 250< lines changed <1000 labels Sep 6, 2023
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Sep 6, 2023
@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (0e80fa5) 97.88% compared to head (6d2e360) 97.88%.
Report is 2 commits behind head on master.

❗ Current head 6d2e360 differs from pull request most recent head 71f5466. Consider uploading reports for the commit 71f5466 to get more accurate results

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     
Files Changed Coverage Δ
cirq-ft/cirq_ft/__init__.py 100.00% <ø> (ø)
cirq-ft/cirq_ft/infra/__init__.py 100.00% <ø> (ø)
cirq-core/cirq/ops/dense_pauli_string.py 97.97% <100.00%> (+0.01%) ⬆️
cirq-core/cirq/ops/linear_combinations.py 99.79% <100.00%> (ø)
cirq-core/cirq/value/value_equality_attr.py 96.87% <100.00%> (+0.26%) ⬆️
cirq-ft/cirq_ft/algos/and_gate_test.py 100.00% <100.00%> (ø)
cirq-ft/cirq_ft/algos/apply_gate_to_lth_target.py 100.00% <100.00%> (ø)
...-ft/cirq_ft/algos/apply_gate_to_lth_target_test.py 100.00% <100.00%> (ø)
cirq-ft/cirq_ft/algos/generic_select.py 100.00% <100.00%> (ø)
cirq-ft/cirq_ft/algos/generic_select_test.py 100.00% <100.00%> (ø)
... and 30 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mpharrigan mpharrigan left a 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:
Copy link
Collaborator

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

Copy link
Collaborator Author

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

good ol' ravel

Comment on lines +375 to +376
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}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@tanujkhattar
Copy link
Collaborator Author

@mpharrigan Addressed your comments. Added a detailed docstring to SelectionRegister explaining usage of ravel / unravel. Enabling auto-merge now.

@tanujkhattar tanujkhattar enabled auto-merge (squash) September 6, 2023 21:35
@tanujkhattar tanujkhattar merged commit 6fae409 into quantumlib:master Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cirq-ft Issues related to the Cirq-FT sub-package BREAKING CHANGE For pull requests that are important to mention in release notes. size: L 250< lines changed <1000

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants