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

Fix dtypes in IntVector and PlusEqualsProduct #1197

Merged
merged 6 commits into from
Jul 27, 2024

Conversation

anurudhp
Copy link
Contributor

@anurudhp anurudhp commented Jul 26, 2024

(part of #1142)

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

I'm surprised this does not raise an error because the call site in the decomposition of QFTPhaseGradient is passing in a register of type QFxp and would need to be updated to include a cast; at the very least.

It's maybe best to open an issue to track the changes needed at the call site and merge this change so we don't go down the rabbit hole. This is a positive improvement so LGTM

@anurudhp
Copy link
Contributor Author

I'm surprised this does not raise an error because the call site in the decomposition of QFTPhaseGradient is passing in a register of type QFxp and would need to be updated to include a cast; at the very least.

I think it's because it uses a cirq style decomp which may not be respecting the dtypes?

def decompose_from_registers(
self, *, context: cirq.DecompositionContext, **quregs: NDArray[cirq.Qid] # type: ignore[type-var]
) -> Iterator[cirq.OP_TREE]:
if self.bitsize == 1:
yield cirq.H(*quregs['q'])
return
q, phase_grad = quregs['q'], quregs['phase_grad']
a, b = q[: self.bitsize // 2], q[self.bitsize // 2 :]
yield QFTPhaseGradient(len(a), False).on_registers(q=a, phase_grad=phase_grad[: len(a)])
yield PlusEqualProduct(len(a), len(b), len(phase_grad)).on_registers(
a=a[::-1], b=b, result=phase_grad
)
yield QFTPhaseGradient(len(b), False).on_registers(q=b, phase_grad=phase_grad[: len(b)])
if self.with_reverse:
for i in range(self.bitsize // 2):
yield cirq.SWAP(q[i], q[-i - 1])

@anurudhp anurudhp enabled auto-merge (squash) July 27, 2024 07:47
@tanujkhattar
Copy link
Collaborator

tanujkhattar commented Jul 27, 2024

I think it's because it uses a cirq style decomp which may not be respecting the dtypes?

I'd be very surprised if that's the root cause.
Update:
I think what's happening is that the
assert_valid_bloq_decomposition(qft_bloq) is testing in the test is wrongly testing the decomposition of TestQFTWithPhaseGradient instead of QFTPhaseGradient.

assert_valid_bloq_decomposition(qft_bloq)

Can you add a test for assert_valid_bloq_decomposition that tests the QFTPhaseGradient bloq instead? I think that test should fail here.


cc @fdmalone Are data type verification checks turned on by default? Do you know what may be going on here?

@anurudhp anurudhp merged commit 70b6dff into quantumlib:main Jul 27, 2024
8 checks passed
@anurudhp anurudhp deleted the 2024/07/26-dtype-fixes branch July 27, 2024 08:12
@fdmalone
Copy link
Collaborator

@tanujkhattar The type checking will only raise an error if things are grossly wrong. By default it's set to LOOSE (which allows a limited set of conversions without a cast from wholly fractional / wholly integer fxps to uints, is that the case here?)
https://github.com/quantumlib/Qualtran/blob/main/qualtran/_infra/data_types.py#L728

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