Moment.resolve_parameter not resolving all the expressions

[ikd-sci]

Description of the issue

How to reproduce the issue

q0 = cirq.LineQubit(0)
c = cirq.Circuit(cirq.Rz(rads=sympy.Mul(sympy.Rational(2, 3), sympy.pi)).on(q0))
cirq.resolve_parameters(c, {'pi': np.pi})

results in:

1: ───Rz(2*pi/3)───

as per discussion with @NoureldinYosri, Moment.resolve_parameter has a bug which can be bypassed by:

def resolve_parameters(circuit, resolver):
    return cirq.Circuit.from_moments(*[[cirq.resolve_parameters(op, resolver) for op in m] for m in circuit])

resolve_parameters(c, {'pi': np.pi})

results in the expected:

1: ───Rz(0.667π)───

Cirq version
'1.5.0.dev'

[dv8081]

@pavoljuhas can you please assign this to me ..

[NoureldinYosri]

@dv8081 the issue that happens is that Moment._resolve_parameters_ checks equality to determine whether to create a new moment or not

Cirq/cirq-core/cirq/circuits/moment.py

Lines 277 to 280 in fd95547

	resolved_op = protocols.resolve_parameters(op, resolver, recursive)
	if resolved_op != op:
	changed = True
	resolved_ops.append(resolved_op)

this doesn't work because symyp.py == np.pi == math.pi so it doesn't do anything. The fix should be to

1. instead of checking equality, check if the new object is the old object with is (i.e. if resolved_op is not op)
2. go through the different gates and ensure that for each gate's _resolve_parameters_ we return self if no change happened and only create a new object if a variable was resolved

[dv8081]

@NoureldinYosri Thanks for pointing this out for me.

[dv8081]

Hi @NoureldinYosri
I have identified several files that contain a resolve_parameters method. Based on your comment I understand that the fix should apply to gates that have this method. I have separated the files into two categories: gates and non-gates.
Gate Files:
parallel_gate.py, phased_x_z_gate.py,phased_iswap_gate.py, diagonal_gate.py, raw_types.py,, three_qubit_gates.py, random_gate_channel.py, fsim_gate.py, controlled_gate.py, wait_gate.py, two_qubit_diagonal_gate.py, eigen_gate.py etc
Non-Gate Files:
dense_pauli_string.py, raw_types.py (second instance), eigen_gate_test.py,controlled_operation.py, classically_controlled_operation.py, pauli_string_phasor.py,gate_operation.py,global_phase_op.py, linear_combinations.py etc
Could you please confirm whether I need to implement the fix for all the gate files? Additionally, should I ignore the non-gate files like dense_pauli_string.py and linear_combinations.py ?

[pavoljuhas]

2. go through the different gates and ensure that for each gate's _resolve_parameters_ we return self if no change happened and only create a new object if a variable was resolved

@NoureldinYosri - Looking at it again, quite a few SomeGate._resolve_parameters_ methods return a new object unconditionally. If we compare the resolved and original gate there, we could hit the same issue with sympy.Float(1) == 1.0 being True and thus return self which has sympy parameters.

Perhaps we can change the fix in Cirq/cirq-core/cirq/circuits/moment.py to

        for op in self:
            resolved_op = protocols.resolve_parameters(op, resolver, recursive)
            changed = (
                changed
                or resolved_op != op
                or (protocols.is_parameterized(op) and not protocols.is_parameterized(resolved_op))
            )
            resolved_ops.append(resolved_op)

This will skip checks for operation change if a changed operation was already found before, and it would detect resolution of sympy constant to python numbers.

[NoureldinYosri]

If we compare the resolved and original gate there, we could hit the same issue with sympy.Float(1) == 1.0

This is why I want to the gates/operations to decide whether a change happended or not without using comparison

protocols.is_parameterized(op) and not protocols.is_parameterized(resolved_op)

This won't work is the resolver is doing a partial resolution (i.e. resolving some parameters and leaving others)

[pavoljuhas]

protocols.is_parameterized(op) and not protocols.is_parameterized(resolved_op)

This won't work is the resolver is doing a partial resolution (i.e. resolving some parameters and leaving others)

The corner case we are trying to solve here is of constant sympy expressions (without any free parameters) getting resolved to Python numbers passing the s_constant == py_number and thus also resolved_op == op equalities. In such case we kept the original op with a sympy constant, but the intent was to resolve it to a Python number. If both op and resolved_op are parameterized, it does not matter that much, because the new moment will in any case have sympy parameters. If op has several sympy parameters, say a, b and only a is resolved, that should also cause the earlier equality to break, ie, resolved_op != op.

I am bit reluctant to make extra changes to gates _resolve_parameters_ methods with a risk of introducing new bugs or corner cases. If you have an example where the fix suggested above breaks, please LMK.

[NoureldinYosri]

@pavoljuhas if you want to just patch the problem, that's fine. but lets keep the issue open

[dv8081]

@pavoljuhas Thank you for reviewing and suggesting the changes. I have committed the updates.

[pavoljuhas]

@pavoljuhas if you want to just patch the problem, that's fine. but lets keep the issue open

The original example bug is fixed by #6794. If there are other cases which still fail, please reopen with such example.