Make a shallow copy of metadata in QuantumCircuit <> DAG conversion#16142
Make a shallow copy of metadata in QuantumCircuit <> DAG conversion#16142matteoacrossi wants to merge 1 commit intoQiskit:mainfrom
metadata in QuantumCircuit <> DAG conversion#16142Conversation
qiskit-bot
added
the
Community PR
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
Thanks for the PR. I left a few comments about why I'm not sure we can unilaterally make this change in #16140 (comment) - I think we're going to need to take a bit more care with API stability, because people could easily be relying on the current behaviour.
It's probably best to keep the conversation over there for the time being, then pivot this PR?
| m.call_method0(intern!(py, "copy")).ok() | ||
| } | ||
| }), | ||
| transpile_layout: ob.getattr(intern!(py, "layout")).ok(), |
There was a problem hiding this comment.
This place (Rust-space extraction of the fields from a Python-space object) is most likely not the right place to put this - that's going to cause the copy in the wrong places, I think. It so happens that circuit_to_dag calls this right now (I think?), but that's just one of the users.
There was a problem hiding this comment.
From #16140 (comment): let's revert this bit, and instead move it into DAGCircuit::from_circuit with a copy gated on copy_op.
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
Thanks for this and the discussion. Can we also add a "performance" release note, and mention the copying behaviour in the documentation of the two conversions?
I can take over the PR to complete it for you if it's getting too fiddly.
| m.call_method0(intern!(py, "copy")).ok() | ||
| } | ||
| }), | ||
| transpile_layout: ob.getattr(intern!(py, "layout")).ok(), |
There was a problem hiding this comment.
From #16140 (comment): let's revert this bit, and instead move it into DAGCircuit::from_circuit with a copy gated on copy_op.
| global_phase=dag.global_phase, | ||
| ) | ||
| circuit.metadata = dag.metadata or {} | ||
| circuit.metadata = dag.metadata.copy() if dag.metadata else {} |
There was a problem hiding this comment.
| circuit.metadata = dag.metadata.copy() if dag.metadata else {} | |
| metadata = dag.metadata or {} | |
| circuit.metadata = metadata.copy() if copy_operations else metadata |
| def test_circuit_to_dag_metadata_is_copied(self): | ||
| """circuit_to_dag should return a DAG with a copy of the metadata, not a reference.""" | ||
| qc = QuantumCircuit(1, metadata={"key": "original"}) | ||
| dag = circuit_to_dag(qc) | ||
| self.assertIsNot(dag.metadata, qc.metadata) | ||
| self.assertEqual(dag.metadata, qc.metadata) | ||
|
|
||
| def test_dag_to_circuit_metadata_is_copied(self): | ||
| """dag_to_circuit should return a circuit with a copy of the metadata, not a reference.""" | ||
| qc = QuantumCircuit(1, metadata={"key": "original"}) | ||
| result = dag_to_circuit(circuit_to_dag(qc)) | ||
| self.assertIsNot(result.metadata, qc.metadata) |
There was a problem hiding this comment.
Given the copying will be conditional, can we do the test explicitly with copy_operations=True?
3 participants
Fixes #10570. Fixes #16140.
As discussed in #10570, metadata is only assigned in
circuit_to_daganddag_to_circuit, resulting in potential side effects when modifying the metadata of one of the circuit instances.This PR implements the change proposed in #10570 to
dag_to_circuit, making a shallow copy of themetadatafield.For
circuit_to_dagthe change is made inconverters.rs, I hope it is the right place.It also adds two quick tests.
AI/LLM disclosure