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

Incorrect on_classical_vals overrides for non classical bloqs #1515

Closed
anurudhp opened this issue Dec 31, 2024 · 8 comments · Fixed by #1518
Closed

Incorrect on_classical_vals overrides for non classical bloqs #1515

anurudhp opened this issue Dec 31, 2024 · 8 comments · Fixed by #1518

Comments

@anurudhp
Copy link
Contributor

I ran into this while implementing #1514. The tensors of these bloq examples seem to have non-trivial phases, which makes them non-classical. The following bloq examples fail in #1514 (I've restricted to bloqs with up to 9 qubits):

sub_from_small

E Mismatched elements: 144 / 65536 (0.22%)
E Max absolute difference: 1.
E Max relative difference: 1.
E x: array([[1., 0., 0., ..., 0., 0., 0.],
E [0., 1., 0., ..., 0., 0., 0.],
E [0., 0., 1., ..., 0., 0., 0.],...
E y: array([[1.+0.j, 0.+0.j, 0.+0.j, ..., 0.+0.j, 0.+0.j, 0.+0.j],
E [0.+0.j, 1.+0.j, 0.+0.j, ..., 0.+0.j, 0.+0.j, 0.+0.j],
E [0.+0.j, 0.+0.j, 1.+0.j, ..., 0.+0.j, 0.+0.j, 0.+0.j],...
cmod_add_k_small

E Mismatched elements: 18 / 1024 (1.76%)
E Max absolute difference: 1.
E Max relative difference: 0.
E x: array([[1., 0., 0., ..., 0., 0., 0.],
E [0., 1., 0., ..., 0., 0., 0.],
E [0., 0., 1., ..., 0., 0., 0.],...
E y: array([[1.+0.j, 0.+0.j, 0.+0.j, ..., 0.+0.j, 0.+0.j, 0.+0.j],
E [0.+0.j, 1.+0.j, 0.+0.j, ..., 0.+0.j, 0.+0.j, 0.+0.j],
E [0.+0.j, 0.+0.j, 1.+0.j, ..., 0.+0.j, 0.+0.j, 0.+0.j],...
moddbl_small

E Mismatched elements: 3 / 256 (1.17%)
E Max absolute difference: 1.
E Max relative difference: 1.
E x: array([[1., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0.],
E [0., 0., 0., 0., 0., 0., 0., 1., 0., 0., 0., 0., 0., 0., 0., 0.],
E [0., 1., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0.],...
E y: array([[1.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j,
E 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j],
E [0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 0.+0.j, 1.+0.j,...
approx_cswap_small

E Mismatched elements: 120 / 262144 (0.0458%)
E Max absolute difference: 2.
E Max relative difference: 2.
E x: array([[1., 0., 0., ..., 0., 0., 0.],
E [0., 1., 0., ..., 0., 0., 0.],
E [0., 0., 1., ..., 0., 0., 0.],...
E y: array([[1.000000e+00+6.471125e-32j, 0.000000e+00+0.000000e+00j,
E 0.000000e+00+0.000000e+00j, ..., 0.000000e+00+0.000000e+00j,
E 0.000000e+00+0.000000e+00j, 0.000000e+00+0.000000e+00j],...
@mpharrigan
Copy link
Collaborator

Do you know where the non-trivial phases are being introduced? Presumably if you write a decomposition of e.g. ModDbl using only classical operations (whose matrices only have +1 entries) then you can't get any phases popping up, right?

@tanujkhattar
Copy link
Collaborator

fyi -- approx_cswap_small's tensor would have a non-trivial phases since it implements an approximate cswap which is correct upto a phase, in order to have 4 T count instead of 7, which gets inverted when the approx cswap inverse is applied. This is similar to the 4 T version of Toffoli which applies a toffoli upto a relative phase (we don't explicitly have a bloq for this yet)

@mpharrigan
Copy link
Collaborator

Yeah, the approx cswap is clearer. It's got "approx" in the name, so it's less surprising that there are phases. The other ones are surprising to me

@NoureldinYosri
Copy link
Contributor

NoureldinYosri commented Jan 8, 2025

sub_from_small => the classical action of SubtractFrom is wrong. it uses fmod which gives wrong answer on overflow instead it should call simulation.classical_sim.add_ints ... this also means that the classical action tests for this bloq are insufficient.

cmod_add_k_small and moddbl_small are because their constructions assume correct range ... that they assume they will be called on states $x < \mod < 2^\mathrm{bitsize}$ ... outside this range (i.e. $\mod \leq x < 2^\mathrm{bitsize}$) which happens for tensor contraction their constructions doen't work .... I can change the on_classical_vals to take this into account (if input is outside the valid range return it as is) ... this will make the tensor contraction work but that may introduce a difference in behaviour between the bloq and it decomposition for those out of bound inputs

@mpharrigan
Copy link
Collaborator

is it obvious how those corner cases lead to phases in the tensors?

@anurudhp
Copy link
Contributor Author

I looked at the failing tests again, and I realized that the arithmetic bloqs fail because the inputs outside range (i.e. >= mod) do not match (the max abs diff is 1, which is from 1s and 0s mismatching).

Only the cswap_approx fails because of a phase mismatch.

@NoureldinYosri
Copy link
Contributor

is it obvious how those corner cases lead to phases in the tensors?

there are no phases except in cswap_approx. as anurudhp this is mismatch is between 0 and 1 for inputs outside the valid range for the bloqs

@mpharrigan
Copy link
Collaborator

ok, that makes way more sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants