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

Make ancilla for BiQubitsMixer sided too. #6313

Merged
merged 2 commits into from Oct 10, 2023

Conversation

fdmalone
Copy link
Contributor

@fdmalone fdmalone commented Oct 9, 2023

Similar fix as #6312 and fixes quantumlib/Qualtran#389

@fdmalone fdmalone requested review from vtomole, cduck and a team as code owners October 9, 2023 22:53
@CirqBot CirqBot added the size: S 10< lines changed <50 label Oct 9, 2023
@tanujkhattar
Copy link
Collaborator

@fdmalone I'm still interested in getting an answer to my question in #6312 (comment), maybe here or in the linked issue quantumlib/Qualtran#389

What's the expected behavior of the ancilla register? Do we need to expose it as a LEFT / RIGHT register at all ?

From a quick glance, it looks like the answer is yes and that the gate is essentially using leaving dirty ancillas in the output, which get cleaned up by its adjoint, in order to reduce the T-counts. If that is the case, it'd be nice to clarify this as part of the docstring.

@tanujkhattar tanujkhattar self-assigned this Oct 9, 2023
@fdmalone
Copy link
Contributor Author

fdmalone commented Oct 9, 2023

I'll defer to @NoureldinYosri

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (fefe350) 97.89% compared to head (b222feb) 97.89%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6313      +/-   ##
==========================================
- Coverage   97.89%   97.89%   -0.01%     
==========================================
  Files        1106     1106              
  Lines       96156    96157       +1     
==========================================
- Hits        94134    94132       -2     
- Misses       2022     2025       +3     
Files Coverage Δ
cirq-ft/cirq_ft/algos/arithmetic_gates.py 99.09% <100.00%> (+<0.01%) ⬆️

... and 2 files with indirect coverage changes

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

@NoureldinYosri
Copy link
Collaborator

"ancilla" here is a misnomer that follows from the original paper. it should be more like target registers that are assumed to be clean.

@tanujkhattar the gate is simillar to cirq_ft.And in the sense that it assumes its target to be clean before it acts ont it and that its adjoint is the way to clean it.

so yea they should be sided (and probably renamed)

[
infra.Register('x', 2),
infra.Register('y', 2),
infra.Register('ancilla', 3, side=one_side),
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets rename it target. this will result in some changes to the decomposition and docstring

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also improve the docstring to more clearly specify what each of the 3 target bits contain. It's not clear to me right now by just reading the docstring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the short answer is .. enough information to do the adjoint. coming to think of it, this is why I kept the register name from the paper as ancilla. because they have no use otherthan as information holders for that will be used by the adjoint.

the actually useful information is stored inplace.

so maybe skip renaming it for now and I will come back for it later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, we've renamed such registers to junk in Qualtran.

@fdmalone As part of this PR, I think we should at least capture this discussion in the docstring and let the reader know that the ancilla is used to reduce the T-counts and assumes that it should be cleaned up later by the adjoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I'll try distill something later today. We should open an issue to track either here or in qualtran.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@fdmalone fdmalone merged commit a98fc61 into quantumlib:master Oct 10, 2023
35 checks passed
@fdmalone fdmalone deleted the add_side_for_biqubits branch October 10, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BiQubitsMixer also can't decompose
4 participants