-
Notifications
You must be signed in to change notification settings - Fork 380
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
fixes #2042 #2308
base: main
Are you sure you want to change the base?
fixes #2042 #2308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking this on yourself. I added a comment on my preferred solution to the bug.
for creg in meas_circuit.cregs: | ||
new_circ.add_register(creg) | ||
if creg.name not in existing_creg_names: # Prevent duplicate registers | ||
new_circ.add_register(creg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good solution since we silently avoid adding some of the registers, leading to unexpected behavior for the user. We should either raise an error with clear description of the name clash or at least output a warning with the same description and rename the measurement register name (I prefer the first solution as I believe it lowers the chance for the user to create code with unexpected results).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the comment and since this is my first contribution I would highly appreciate it if I could get any feedback. I have written two solutions to this (with different approaches). Could you tell me which one is better to use in this repository?
1. Raising the Error
for meas_circuit in meas_circuits:
new_circ = circuit.copy()
existing_creg_names = {creg.name for creg in new_circ.cregs}
for creg in meas_circuit.cregs:
# If there's a register name clash, raise a clear error
if creg.name in existing_creg_names:
raise QiskitError(
f"Cannot add measurement register '{creg.name}' because its name "
"already exists in the circuit. Please rename or remove duplicates."
)
new_circ.add_register(creg)
2. Auto-renaming with a Suffix
for meas_circuit in meas_circuits:
new_circ = circuit.copy()
existing_creg_names = {creg.name for creg in new_circ.cregs}
for creg in meas_circuit.cregs:
new_name = creg.name
# If there's a register name clash, rename and warn
if new_name in existing_creg_names:
warnings.warn(
f"Register name '{new_name}' already exists. Automatically "
f"renaming measurement register to avoid conflicts.",
UserWarning,
)
suffix = 1
while new_name in existing_creg_names:
new_name = f"{creg.name}_{suffix}"
suffix += 1
new_circ.add_register(ClassicalRegister(creg.size, new_name))
Greetings,
This is my first issue.
Summary
Fixed issue #2042 by adding a check for estimator adding duplicate classical registers in _combine_circs.
Details and comments
I added a check to _combinecircs in qiskit_aer > primitives > estimator.py to prevent a clash of register names for estimator which happens if the same name is used e.g. 'c'.