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

Error simulating circuit with condition based on more than two bits #215

Closed
cqc-alec opened this issue Aug 14, 2024 · 6 comments · Fixed by #224
Closed

Error simulating circuit with condition based on more than two bits #215

cqc-alec opened this issue Aug 14, 2024 · 6 comments · Fixed by #224
Assignees
Labels
bug Something isn't working

Comments

@cqc-alec
Copy link
Collaborator

Describe the bug
I am not sure if this is a bug in pytket-phir or pecos. When an operation in a circuit is conditional on the values of more than two bits, the generated PHIR contains a section like this:

        {
            "block": "if",
            "condition":
            {
                "cop": "&",
                "args":
                [
                    {
                        "cop": "==",
                        "args":
                        [
                            [
                                "c",
                                2
                            ],
                            0
                        ]
                    },
                    {
                        "cop": "==",
                        "args":
                        [
                            [
                                "c",
                                1
                            ],
                            0
                        ]
                    },
                    {
                        "cop": "==",
                        "args":
                        [
                            [
                                "c",
                                0
                            ],
                            0
                        ]
                    }
                ]
            },
            "true_branch":
            [
                {
                    "qop": "RZ",
                    "angles":
                    [
                        [
                            0.5
                        ],
                        "pi"
                    ],
                    "args":
                    [
                        [
                            "q",
                            0
                        ]
                    ]
                }
            ]
        }

When this PHIR is passed to the simulation engine, we get an error:

Traceback (most recent call last):
  File "/home/alec/r/pytket-quantinuum/silas.py", line 19, in <module>
    res = engine.run(phir, shots=10)
  File "/home/alec/r/pytket-quantinuum/env/lib/python3.10/site-packages/pecos/engines/hybrid_engine.py", line 175, in run
    for buffered_ops in self.cinterp.execute(self.cinterp.program.ops):
  File "/home/alec/r/pytket-quantinuum/env/lib/python3.10/site-packages/pecos/classical_interpreters/phir_classical_interpreter.py", line 164, in execute
    for op in self._flatten_blocks(seq):
  File "/home/alec/r/pytket-quantinuum/env/lib/python3.10/site-packages/pecos/classical_interpreters/phir_classical_interpreter.py", line 149, in _flatten_blocks
    if self.eval_expr(op.condition):
  File "/home/alec/r/pytket-quantinuum/env/lib/python3.10/site-packages/pecos/classical_interpreters/phir_classical_interpreter.py", line 216, in eval_expr
    lhs, rhs = args
ValueError: too many values to unpack (expected 2)

Somewhere in the stack there seems to be an assumption that the & operator must have 2 arguments.

To Reproduce

from pytket.circuit import Bit, Circuit, Qubit
from pytket.phir.api import pytket_to_phir
from pecos.engines.hybrid_engine import HybridEngine

n_bits = 3
c = Circuit(1, n_bits)
c.Rz(0.5, 0, condition_bits=list(range(n_bits)), condition_value=0)
phir = pytket_to_phir(c)
engine = HybridEngine(qsim="state-vector")
engine.use_seed(0)
res = engine.run(phir, shots=10)

Expected behavior
The circuit should be simulated without error.

Additional context
Issue originally raised here: CQCL/pytket-pecos#39 .

@cqc-alec cqc-alec added the bug Something isn't working label Aug 14, 2024
@qartik
Copy link
Member

qartik commented Aug 15, 2024

The PHIR generated does look okay to me:

PHIRModel(
    format_='PHIR/JSON',
    version='0.1.0',
    metadata={'source': 'pytket-phir v0.8.1.dev22'},
    ops=[
        QVarDefine(metadata=None, data='qvar_define', data_type='qubits', variable='q', size=1),
        CVarDefine(metadata=None, data='cvar_define', data_type='i64', variable='c', size=3),
        Comment(c='IF ([c[0], c[1], c[2]] == 0) THEN Rz(0.5) q[0];'),
        IfBlock(
            metadata=None,
            block='if',
            condition=COp(
                metadata=None,
                cop='&',
                returns=None,
                args=[
                    COp(metadata=None, cop='==', returns=None, args=[('c', 2), 0]),
                    COp(metadata=None, cop='==', returns=None, args=[('c', 1), 0]),
                    COp(metadata=None, cop='==', returns=None, args=[('c', 0), 0])
                ]
            ),
            true_branch=[SQOp(metadata=None, qop='RZ', angles=([0.5], 'pi'), args=[('q', 0)])],
            false_branch=None
        )
    ]
)

The error seems like it is in PECOS as it doesn't seem to be accounting for the AND of conditions:

self = <pecos.classical_interpreters.phir_classical_interpreter.PHIRClassicalInterpreter object at 0x130a55280>, expr = <pecos.reps.pypmir.op_types.COp object at 0x130d027e0>

    def eval_expr(self, expr: int | str | list | pt.opt.COp) -> int | None:
        """Evaluates integer expressions."""
        match expr:
            case int():
                return expr

            case str():
                return self.get_cval(expr)
            case list():
                return self.get_bit(*expr)
            case pt.opt.COp():
                sym = expr.name
                args = expr.args

                if sym in {"~"}:  # Unary ops
                    lhs = args[0]
                    rhs = None
                else:
>                   lhs, rhs = args
E                   ValueError: too many values to unpack (expected 2)

.venv/lib/python3.12/site-packages/pecos/classical_interpreters/phir_classical_interpreter.py:216: ValueError

@qciaran could you confirm?

@cqc-alec
Copy link
Collaborator Author

Yes, I thought this might be a pecos issue but wasn't entirely sure whether the intended semantics of the cop='&' covered the case of more than two arguments.

@qciaran
Copy link
Collaborator

qciaran commented Aug 20, 2024

Yeah, I would think an & that contains anything but two arguments would be wrong... I intended & to be a pure bitwise binary operations. So a & b or a[0] & b[1] is valid but &(a[0], b[1], c[2]) is not. That is interesting to consider. But given the current syntax... If the entire register can't be used, such as if(c==0), then I would imagine you would want (c[0] == 0) & (c[1] == 0) & (c[2] == 0) or equivalently (((c[0] == 0) & (c[1] == 0)) & (c[2] == 0)).

@qciaran
Copy link
Collaborator

qciaran commented Aug 20, 2024

I have wondered if we should introduce slices of some sort...

@qartik
Copy link
Member

qartik commented Aug 23, 2024

If the entire register can't be used, such as if(c==0)

If this is a common use case (which seems true to me), I am happy to change phirgen to try to generate register-level condition. I did something similar for MultiBitOp at https://github.com/CQCL/pytket-phir/blob/main/pytket/phir/phirgen.py#L320-L332

The question of allowing only two arguments with classical ops vs more remains. I am happy either way -- either phirgen can produce nested binary expressions or PECOS can consider supporting multiple arguments.

@qartik qartik self-assigned this Aug 27, 2024
@qciaran
Copy link
Collaborator

qciaran commented Sep 3, 2024

I think for now, if greater than two things do need ANDing then it should be done so in using the nested binary approach.

Yeah, if this is just representing an entire register and is being broken down into bits, it might be nice to generate the register-level condition as you say.

As far as bit ANDing and other ways to express it, I think I need more time to think if there is an alternative way of expressing it that I am comfortable with. And it might require a decent change to the PHIR spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants