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

Delay.__eq__ does not compare the delay unit #13812

Closed
wshanks opened this issue Feb 8, 2025 · 4 comments · Fixed by #13816
Closed

Delay.__eq__ does not compare the delay unit #13812

wshanks opened this issue Feb 8, 2025 · 4 comments · Fixed by #13816
Labels
bug Something isn't working
Milestone

Comments

@wshanks
Copy link
Contributor

wshanks commented Feb 8, 2025

Environment

  • Qiskit version: 1.3.2 or 9ae0a80
  • Python version: 3.11
  • Operating system: Fedora Linux 41

What is happening?

When comparing two Delay instructions, only the duration argument to __init__ matters, not the unit argument.

How can we reproduce the issue?

from qiskit import QuantumCircuit
from qiskit.circuit.delay import Delay

qc1 = QuantumCircuit(1)
qc1.delay(1, 0, unit="s")

qc2 = QuantumCircuit(1)
qc2.delay(1, 0, unit="dt")

print("Compare circuits:", qc1 == qc2)
print("Compare delays:", Delay(1, "dt") == Delay(1, "s"))

What should happen?

The code above should print False/False but it prints False/True.

Any suggestions?

Before #13486, both the circuit and instruction comparisons above were true. Now the circuit one is false. This came up in Qiskit Experiments because of this other bug with delay units and qpy. A test that ran a circuit with a delay roundtrip through qpy that used to pass even though the delay unit was lost started to fail.

@jakelishman
Copy link
Member

Thanks Will. For the immediate period, I've left the non-equality of 1000ms and 1s in place, since it's not clear to me whether a user might expect those to compare equal (why would they have specified them with different units?) and it's faffier to make work in the current Rust structure, but #13816 will fix your s/dt comparisons.

@wshanks
Copy link
Contributor Author

wshanks commented Feb 10, 2025

Thanks, @jakelishman! Do you happen to know what happens with the circuit case now? My guess was that delay gets turned into a Rust instruction object that does consider the unit, but I didn't trace it down enough to tell. Does #13816 align the Python class comparison with the circuit one?

@jakelishman
Copy link
Member

When we bring the Delay down to a Rust object to add to a circuit, we encode its unit into a Rust enum locally. Part of the Rust-space comparison (implicitly) compares the unit values for equality. We don't recreate or use Python-space objects in the circuit equality checks if we can avoid it (even if they already exist), which is how the discrepancy happened.

@jakelishman
Copy link
Member

And yeah, #13816 makes the two behaviours match, but they're not calling the same code (which would be a mess to do from Python, and wildly expensive from Rust), so it's just that the same logic is implemented twice. It's not pretty, but it's the same as happens with gates, and tbh, a lot of Qiskit's equality model is kind of messy at best, and broken at worst.

Part of the reason I didn't do the unit-conversion comparisons for s/ms/etc is because it'd be ugly to achieve with both the current Rust and Python structures.

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