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

Port VF2 to rustworkx-core #1235

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

kevinhartman
Copy link
Contributor

@kevinhartman kevinhartman commented Jul 1, 2024

Summary

Ports Rustworkx's modified VF2 algorithm to rustworkx-core, supporting generic petgraph types like StableGraph.

Details

This implementation should work for petgraph's StableGraph, Graph, and GraphMap (though IIRC there is a bug in petgraph's implementation of their EdgeCount trait for GraphMap which would likely cause issues until it gets fixed edit: I believe it only triggers if edges are removed, so it'd likely be fine).

In contrast with the original petgraph::algo::isomorphism API which hides the fact that VF2 is used under the hood for isomorphism, this PR introduces the VF2 internals as a public interface (i.e. rustworkx_core::isomorphism::vf2), exposing traits NodeMatcher and EdgeMatcher and structs Vf2Algorithm and Vf2State, which can be used to further customize how VF2 is executed. We now use this in our Python API's implementation of (Di|)GraphVF2Mapping, for example. (...the only sort of "kludge" regarding the above is that I had to make a few fields on Vf2Algorithm and Vf2State public in order to support Python garbage collection requirements. If you've got a better idea, please leave a comment.)

Changes from the Python impl

  • To support generic fail-able node and edge matching, the error type IsIsomorphicError is introduced, which encapsulates the error types returned by the user provided NodeMatcher and EdgeMatcher.
  • Vf2Algorithm now formally implements Iterator, with an Item type of Result.

To-do

  • Release note.
  • Fix edge matcher regression (I'm aware of the issue, working on fix).
  • Add more detailed PR description 🙂.

@kevinhartman kevinhartman marked this pull request as ready for review July 1, 2024 22:29
@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9751462927

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 954 of 1063 (89.75%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 95.412%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/isomorphism/mod.rs 82 100 82.0%
rustworkx-core/src/isomorphism/vf2.rs 830 868 95.62%
src/isomorphism/vf2_mapping.rs 42 95 44.21%
Totals Coverage Status
Change from base Build 9718474595: -0.4%
Covered Lines: 18280
Relevant Lines: 19159

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9755097325

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 954 of 1063 (89.75%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 95.412%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/isomorphism/mod.rs 82 100 82.0%
rustworkx-core/src/isomorphism/vf2.rs 830 868 95.62%
src/isomorphism/vf2_mapping.rs 42 95 44.21%
Totals Coverage Status
Change from base Build 9718474595: -0.4%
Covered Lines: 18280
Relevant Lines: 19159

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9765697514

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 954 of 1063 (89.75%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 95.412%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/isomorphism/mod.rs 82 100 82.0%
rustworkx-core/src/isomorphism/vf2.rs 830 868 95.62%
src/isomorphism/vf2_mapping.rs 42 95 44.21%
Totals Coverage Status
Change from base Build 9718474595: -0.4%
Covered Lines: 18280
Relevant Lines: 19159

💛 - Coveralls

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Georgios would be the best person to review this but I will have to step in. Overall the rustworkx-core looks like a clean port! 10/10, I think we might get more Rust users for rustworkx-core as Isomorphism is one of the methods that differentiate our library.

However, I do not want to use Python::assume_gil_acquired(). So far we have always let PyO3 handle all the uncertainities in the Python FFI, we have zero unsafe code in Rustworkx directly. Can't we use Python::with_gil or some type of closure? There has to be a better way

n1: NodeIndex,
) -> Result<bool, Self::Error> {
if let (Some(a), Some(b)) = (g0.node_weight(n0), g1.node_weight(n1)) {
unsafe {
Copy link
Collaborator

@IvanIsCoding IvanIsCoding Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because PyO3 lets us do this does not mean we should take it. I think it is a big amount of risk for a refactor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Ivan here, this is not a good idea it's not just unsafe in the keyword but also I don't think the assertion around gil ownership is accurate. Are you sure this is true in the presence of python threads? I think this would be safe in the case of single threaded python code, but if we start accounting for multiple threads around vf2_mapping I don't believe it is.

I think we should be usingwith_gil or similar here, and then see what the runtime overhead is using it. If it's significant we can revisit other strategies for the python side of the library.

@mtreinish mtreinish linked an issue Jul 30, 2024 that may be closed by this pull request
@kevinhartman
Copy link
Contributor Author

Thanks for the review, @IvanIsCoding!

Regarding the Python GIL acquisition in the PyMatcher implementations of NodeMatcher and EdgeMatcher, I think assuming the GIL is acquired is the best thing we can do.

The main issue with threading the actual GIL into those callbacks is that the callbacks get moved into the (Di|)GraphVF2Mapping pyclass structs, which means they can't have any lifetime parameters (pyclass structs cannot have lifetime parameters in PyO3) and thus can't hold a Python<'py> token. In the original Python-only code, this was worked around by having the next method on VF2Mapping take a Python<'py> token, which was then threaded down to the callbacks when pumping the iterator. But this isn't appropriate in rustworkx-core, of course.

We certainly could use Python::with_gil instead of Python::assume_gil_acquired, but my expectation is that this would introduce a performance regression, since PyO3 would be checking for the GIL for every single comparison, be it a node or an edge.

Given that the matchers are only invoked while we're pumping the iterator from Python, we should be safe to assume the GIL is acquired, as long as we aren't using Rayon (or other multithreading) to perform matching on other threads.

@IvanIsCoding
Copy link
Collaborator

I think unsafe as a last-resource is OK and with Matthew's +1 we can merge it as is.

However, before we do that: can you try using a PyFunction similar to how we use it in here:

pub enum CostFn {
?

We pass PyFunction's to Dijkstra. Couldn't we do the same here? Then the calls would be fn.call1((a, b)) instead of requiring the py argument

mtreinish added a commit to kevinhartman/qiskit that referenced this pull request Aug 12, 2024
This commit migrates the entirety of the `DAGCircuit` class to Rust. It
fully replaces the Python version of the class. The primary advantage
of this migration is moving from a Python space rustworkx directed graph
representation to a Rust space petgraph (the upstream library for
rustworkx) directed graph. Moving the graph data structure to rust
enables us to directly interact with the DAG directly from transpiler
passes in Rust in the future. This will enable a significant speed-up in
those transpiler passes. Additionally, this should also improve the
memory footprint as the DAGCircuit no longer stores `DAGNode`
instances, and instead stores a lighter enum NodeType, which simply
contains a `PackedInstruction` or the wire objects directly.

Internally, the new Rust-based `DAGCircuit` uses a `petgraph::StableGraph`
with node weights of type `NodeType` and edge weights of type `Wire`. The
NodeType enum contains variants for `QubitIn`, `QubitOut`, `ClbitIn`,
`ClbitOut`, and `Operation`, which should save us from all of the
`isinstance` checking previously needed when working with `DAGNode` Python
instances. The `Wire` enum contains variants `Qubit`, `Clbit`, and `Var`.

As the full Qiskit data model is not rust-native at this point while
all the class code in the `DAGCircuit` exists in Rust now, there are
still sections that rely on Python or actively run Python code via Rust
to function. These typically involve anything that uses `condition`,
control flow, classical vars, calibrations, bit/register manipulation,
etc. In the future as we either migrate this functionality to Rust or
deprecate and remove it this can be updated in place to avoid the use
of Python.

API access from Python-space remains in terms of `DAGNode` instances to
maintain API compatibility with the Python implementation. However,
internally, we convert to and deal in terms of NodeType. When the user
requests a particular node via lookup or iteration, we inflate an ephemeral
`DAGNode` based on the internal `NodeType` and give them that. This is very
similar to what was done in Qiskit#10827 when porting CircuitData to Rust.

As part of this porting there are a few small differences to keep in
mind with the new Rust implementation of DAGCircuit. The first is that
the topological ordering is slightly different with the new DAGCircuit.
Previously, the Python version of `DAGCircuit` using a lexicographical
topological sort key which was basically `"0,1,0,2"` where the first
`0,1` are qargs on qubit indices `0,1` for nodes and `0,2` are cargs
on clbit indices `0,2`. However, the sort key has now changed to be
`(&[Qubit(0), Qubit(1)], &[Clbit(0), Clbit(2)])` in rust in this case
which for the most part should behave identically, but there are some
edge cases that will appear where the sort order is different. It will
always be a valid topological ordering as the lexicographical key is
used as a tie breaker when generating a topological sort. But if you're
relaying on the exact same sort order there will be differences after
this PR. The second is that a lot of undocumented functionality in the
DAGCircuit which previously worked because of Python's implicit support
for interacting with data structures is no longer functional. For
example, previously the `DAGCircuit.qubits` list could be set directly
(as the circuit visualizers previously did), but this was never
documented as supported (and would corrupt the DAGCircuit). Any
functionality like this we'd have to explicit include in the Rust
implementation and as they were not included in the documented public
API this PR opted to remove the vast majority of this type of
functionality.

The last related thing might require future work to mitigate is that
this PR breaks the linkage between `DAGNode` and the underlying
`DAGCirucit` object. In the Python implementation the `DAGNode` objects
were stored directly in the `DAGCircuit` and when an API method returned
a `DAGNode` from the DAG it was a shared reference to the underlying
object in the `DAGCircuit`. This meant if you mutated the `DAGNode` it
would be reflected in the `DAGCircuit`. This was not always a sound
usage of the API as the `DAGCircuit` was implicitly caching many
attributes of the DAG and you should always be using the `DAGCircuit`
API to mutate any nodes to prevent any corruption of the `DAGCircuit`.
However, now as the underlying data store for nodes in the DAG are
no longer the python space objects returned by `DAGCircuit` methods
mutating a `DAGNode` will not make any change in the underlying
`DAGCircuit`. This can come as quite the surprise at first, especially
if you were relying on this side effect, even if it was unsound.

It's also worth noting that 2 large pieces of functionality from
rustworkx are included in this PR. These are the new files
`rustworkx_core_vnext` and `dot_utils` which are rustworkx's VF2
implementation and its dot file generation. As there was not a rust
interface exposed for this functionality from rustworkx-core there was
no way to use these functions in rustworkx. Until these interfaces
added to rustworkx-core in future releases we'll have to keep these
local copies. The vf2 implementation is in progress in
Qiskit/rustworkx#1235, but `dot_utils` might make sense to keep around
longer term as it is slightly modified from the upstream rustworkx
implementation to directly interface with `DAGCircuit` instead of a
generic graph.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
@kevinhartman
Copy link
Contributor Author

Thanks for the additional review @IvanIsCoding!

We pass PyFunction's to Dijkstra. Couldn't we do the same here? Then the calls would be fn.call1((a, b)) instead of requiring the py argument

I'm not sure I'm finding / understanding exactly what you're describing here. It looks like CostFn is an enum that can be either a default f64 value or a PyObject callable from the user. In the impl for CostFn, the call method appears to require a py token.

I think the only way to really get a Python token into the matchers would be to remove them from the VF2Algorithm struct and pass them in to the call to next every time, i.e. inject the Python token into node_matcher and edge_matcher closures and use a custom next method that takes them as parameters. But, that feels like a bad API design to me, since it allows the user to change the definition of what constitutes a match with every pump of the "iterator" (in quotes because we'd also no longer be able to implement Iterator since we'd need a custom signature for next).

Let me know if I'm misunderstanding the example you mentioned. It'd be neat to get things working without unsafe, but I'm not optimistic it is possible without making serious concessions to the integrity of the Rust API for this. And, I'm not worried about what we're doing here with unsafe because it is tightly controlled and entirely encapsulated from our users.

github-merge-queue bot pushed a commit to Qiskit/qiskit that referenced this pull request Aug 23, 2024
* Port DAGCircuit to Rust

This commit migrates the entirety of the `DAGCircuit` class to Rust. It
fully replaces the Python version of the class. The primary advantage
of this migration is moving from a Python space rustworkx directed graph
representation to a Rust space petgraph (the upstream library for
rustworkx) directed graph. Moving the graph data structure to rust
enables us to directly interact with the DAG directly from transpiler
passes in Rust in the future. This will enable a significant speed-up in
those transpiler passes. Additionally, this should also improve the
memory footprint as the DAGCircuit no longer stores `DAGNode`
instances, and instead stores a lighter enum NodeType, which simply
contains a `PackedInstruction` or the wire objects directly.

Internally, the new Rust-based `DAGCircuit` uses a `petgraph::StableGraph`
with node weights of type `NodeType` and edge weights of type `Wire`. The
NodeType enum contains variants for `QubitIn`, `QubitOut`, `ClbitIn`,
`ClbitOut`, and `Operation`, which should save us from all of the
`isinstance` checking previously needed when working with `DAGNode` Python
instances. The `Wire` enum contains variants `Qubit`, `Clbit`, and `Var`.

As the full Qiskit data model is not rust-native at this point while
all the class code in the `DAGCircuit` exists in Rust now, there are
still sections that rely on Python or actively run Python code via Rust
to function. These typically involve anything that uses `condition`,
control flow, classical vars, calibrations, bit/register manipulation,
etc. In the future as we either migrate this functionality to Rust or
deprecate and remove it this can be updated in place to avoid the use
of Python.

API access from Python-space remains in terms of `DAGNode` instances to
maintain API compatibility with the Python implementation. However,
internally, we convert to and deal in terms of NodeType. When the user
requests a particular node via lookup or iteration, we inflate an ephemeral
`DAGNode` based on the internal `NodeType` and give them that. This is very
similar to what was done in #10827 when porting CircuitData to Rust.

As part of this porting there are a few small differences to keep in
mind with the new Rust implementation of DAGCircuit. The first is that
the topological ordering is slightly different with the new DAGCircuit.
Previously, the Python version of `DAGCircuit` using a lexicographical
topological sort key which was basically `"0,1,0,2"` where the first
`0,1` are qargs on qubit indices `0,1` for nodes and `0,2` are cargs
on clbit indices `0,2`. However, the sort key has now changed to be
`(&[Qubit(0), Qubit(1)], &[Clbit(0), Clbit(2)])` in rust in this case
which for the most part should behave identically, but there are some
edge cases that will appear where the sort order is different. It will
always be a valid topological ordering as the lexicographical key is
used as a tie breaker when generating a topological sort. But if you're
relaying on the exact same sort order there will be differences after
this PR. The second is that a lot of undocumented functionality in the
DAGCircuit which previously worked because of Python's implicit support
for interacting with data structures is no longer functional. For
example, previously the `DAGCircuit.qubits` list could be set directly
(as the circuit visualizers previously did), but this was never
documented as supported (and would corrupt the DAGCircuit). Any
functionality like this we'd have to explicit include in the Rust
implementation and as they were not included in the documented public
API this PR opted to remove the vast majority of this type of
functionality.

The last related thing might require future work to mitigate is that
this PR breaks the linkage between `DAGNode` and the underlying
`DAGCirucit` object. In the Python implementation the `DAGNode` objects
were stored directly in the `DAGCircuit` and when an API method returned
a `DAGNode` from the DAG it was a shared reference to the underlying
object in the `DAGCircuit`. This meant if you mutated the `DAGNode` it
would be reflected in the `DAGCircuit`. This was not always a sound
usage of the API as the `DAGCircuit` was implicitly caching many
attributes of the DAG and you should always be using the `DAGCircuit`
API to mutate any nodes to prevent any corruption of the `DAGCircuit`.
However, now as the underlying data store for nodes in the DAG are
no longer the python space objects returned by `DAGCircuit` methods
mutating a `DAGNode` will not make any change in the underlying
`DAGCircuit`. This can come as quite the surprise at first, especially
if you were relying on this side effect, even if it was unsound.

It's also worth noting that 2 large pieces of functionality from
rustworkx are included in this PR. These are the new files
`rustworkx_core_vnext` and `dot_utils` which are rustworkx's VF2
implementation and its dot file generation. As there was not a rust
interface exposed for this functionality from rustworkx-core there was
no way to use these functions in rustworkx. Until these interfaces
added to rustworkx-core in future releases we'll have to keep these
local copies. The vf2 implementation is in progress in
Qiskit/rustworkx#1235, but `dot_utils` might make sense to keep around
longer term as it is slightly modified from the upstream rustworkx
implementation to directly interface with `DAGCircuit` instead of a
generic graph.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* Update visual mpl circuit drawer references

Right now there is a bug in the matplotlib circuit visualizer likely
caused by the new `__eq__` implementation for `DAGOpNode` that didn't
exist before were some gates are missing from the visualization. In the
interest of unblocking this PR this commit updates the references for
these cases temporarily until this issue is fixed.

* Ensure DAGNode.sort_key is always a string

Previously the sort_key attribute of the Python space DAGCircuit was
incorrectly being set to `None` for rust generated node objects. This
was done as for the default path the sort key is determined from the
rust domain's representation of qubits and there is no analogous data in
the Python object. However, this was indavertandly a breaking API change
as sort_key is expected to always be a string. This commit adds a
default string to use for all node types so that we always have a
reasonable value that matches the typing of the class. A future step is
likely to add back the `dag` kwarg to the node types and generate the
string on the fly from the rust space data.

* Make Python argument first in Param::eq and Param::is_close

The standard function signature convention for functions that take a
`py: Python` argument is to make the Python argument the first (or
second after `&self`). The `Param::eq` and `Param::is_close` methods
were not following this convention and had `py` as a later argument in
the signature. This commit corrects the oversight.

* Fix merge conflict with #12943

With the recent merge with main we pulled in #12943 which conflicted
with the rust space API changes made in this PR branch. This commit
updates the usage to conform with the new interface introduced in this
PR.

* Add release notes and test for invalid args on apply methods

This commit adds several release notes to document this change. This
includes a feature note to describe the high level change and the user
facing benefit (mainly reduced memory consumption for DAGCircuits),
two upgrade notes to document the differences with shared references
caused by the new data structure, and a fix note documenting the fix
for how qargs and cargs are handled on `.apply_operation_back()` and
`.apply_operation_front()`. Along with the fix note a new unit test is
added to serve as a regression test so that we don't accidentally allow
adding cargs as qargs and vice versa in the future.

* Restore `inplace` argument functionality for substitute_node()

This commit restores the functionality of the `inplace` argument for
`substitute_node()` and restores the tests validating the object
identity when using the flag. This flag was originally excluded from
the implementation because the Rust representation of the dag is not
a shared reference with Python space and the flag doesn't really mean
the same thing as there is always a second copy of the data for Python
space now. The implementation here is cheating slighty as we're passed
in the DAG node by reference it relies on that reference to update the
input node at the same time we update the dag. Unlike the previous
Python implementation where we were updating the node in place and the
`inplace` argument was slightly faster because everything was done by
reference. The rust space data is still a compressed copy of the data
we return to Python so the `inplace` flag will be slightly more
inefficient as we need to copy to update the Python space representation
in addition to the rust version.

* Revert needless dict() cast on metadata in dag_to_circuit()

This commit removes an unecessary `dict()` cast on the `dag.metadata`
when setting it on `QuantumCircuit.metadata` in
`qiskit.converters.dag_to_circuit()`. This slipped in at some point
during the development of this PR and it's not clear why, but it isn't
needed so this removes it.

* Add code comment for DAGOpNode.__eq__ parameter checking

This commit adds a small inline code comment to make it clear why we
skip parameter comparisons in DAGOpNode.__eq__ for python ops. It might
not be clear why the value is hard coded to `true` in this case, as this
check is done via Python so we don't need to duplicate it in rust space.

* Raise a ValueError on DAGNode creation with invalid index

This commit adds error checking to the DAGNode constructor to raise a
PyValueError if the input index is not valid (any index < -1).
Previously this would have panicked instead of raising a user catchable
error.

* Use macro argument to set python getter/setter name

This commit updates the function names for `get__node_id` and
`set__node_id` method to use a name that clippy is happy with and
leverage the pyo3 macros to set the python space name correctly instead
of using the implicit naming rules.

* Remove Ord and PartialOrd derives from interner::Index

The Ord and PartialOrd traits were originally added to the Index struct
so they could be used for the sort key in lexicographical topological
sorting. However, that approach was abandonded during the development of
this PR and instead the expanded Qubit and Clbit indices were used
instead. This left the ordering traits as unnecessary on Index and
potentially misleading. This commit just opts to remove them as they're
not needed anymore.

* Fix missing nodes in matplotlib drawer.

Previously, the change in equality for DAGNodes was causing nodes
to clobber eachother in the matplotlib drawer's tracking data
structures when used as keys to maps.

To fix this, we ensure that all nodes have a unique ID across
layers before constructing the matplotlib drawer. They actually
of course _do_ in the original DAG, but we don't really care
what the original IDs are, so we just make them up.

Writing to _node_id on a DAGNode may seem odd, but it exists
in the old Python API (prior to being ported to Rust) and
doesn't actually mutate the DAG at all since DAGNodes are
ephemeral.

* Revert "Update visual mpl circuit drawer references"

With the previous commit the bug in the matplotlib drawer causing the
images to diverge should be fixed. This commit reverts the change to the
reference images as there should be no difference now.

This reverts commit 1e4e6f3.

* Update visual mpl circuit drawer references for control flow circuits

The earlier commit that "fixed" the drawers corrected the visualization
to match expectations in most cases. However after restoring the
references to what's on main several comparison tests with control flow
in the circuit were still failing. The failure mode looks similar to the
other cases, but across control flow blocks instead of at the circuit
level. This commit temporarily updates the references of these to the
state of what is generated currently to unblock CI. If/when we have a
fix this commit can be reverted.

* Fix edge cases in DAGOpNode.__eq__

This commit fixes a couple of edge cases in DAGOpNode.__eq__ method
around the python interaction for the method. The first is that in
the case where we had python object parameter types for the gates we
weren't comparing them at all. This is fixed so we use python object
equality for the params in this case. Then we were dropping the error
handling in the case of using python for equality, this fixes it to
return the error to users if the equality check fails. Finally a comment
is added to explain the expected use case for `DAGOpNode.__eq__` and why
parameter checking is more strict than elsewhere.

* Remove Param::add() for global phase addition

This commit removes the Param::add() method and instead adds a local
private function to the `dag_circuit` module for doing global phase
addition. Previously the `Param::add()` method was used solely for
adding global phase in `DAGCircuit` and it took some shortcuts knowing
that context. This made the method implementation ill suited as a
general implementation.

* More complete fix for matplotlib drawer.

* Revert "Update visual mpl circuit drawer references for control flow circuits"

This reverts commit 9a6f953.

* Unify rayon versions in workspace

* Remove unused _GLOBAL_NID.

* Use global monotonic ID counter for ids in drawer

The fundamental issue with matplotlib visualizations of control flow is
that locally in the control flow block the nodes look the same but are
stored in an outer circuit dictionary. If the gates are the same and on
the same qubits and happen to have the same node id inside the different
control flow blocks the drawer would think it's already drawn the node
and skip it incorrectly. The previous fix for this didn't go far enough
because it wasn't accounting for the recursive execution of the drawer
for inner blocks (it also didn't account for LayerSpoolers of the same
length).

* Re-add missing documentation

* Remove unused BitData iterator stuff.

* Make types, dag, and bit count methods public

This commit makes some attributes of the dag circuit public as they will
need to be accessible from the accelerate crate to realistically start
using the DAGCircuit for rust transpiler passes.

* Make Wire pickle serialization explicit

This commit pivots away from using the PyO3 crate's conversion traits
for specialized pickle serialization output of Wire objects. The output
of the previous traits wasn't really intended for representing a Wire in
Python but only for pickle serialization. This commit migrates these to
custom methods, without a trait, to make it clear they're only for
pickle.

* Make py token usage explicit in _VarIndexMap

The _VarIndexMap type was designed to look like an IndexMap but is
actually an inner python dictionary. This is because `Var` types are still
defined in python and we need to use a dictionary if we have `Var`
objects as keys in the mapping. In the interest of looking like an
IndexMap all the methods (except for 1) used `with_gil` internally to
work with the dictionary. This could add unecessary overhead and to make
it explicit that there is python involvement with this struct's methods
this commit adds a py: Python argument to all the methods and removes
the `with_gil` usage.

* Make all pub(crate) visibility pub

* Remove unused method

* Reorganize code structure around PyVariableMapper and BitLocations

* Add missing var wires to .get_wires() method

In the porting of the get_wires() method to Rust the handling of Var
wires was missed in the output of the method. This commit corrects the
oversight and adds them to the output.

* Raise TypeError not ValueError for invalid input to set_global_phase

* De-duplicate check logic for op node adding methods

The methods for checking the input was valid on apply_operation_back,
apply_operation_front, and _apply_op_node_back were all identical. This
combines them into a single method to deduplicate the code.

* Improve collect_1q_runs() filter function

The filter function for collect_1q_runs() was needlessly building a
matrix for all the standard gates when all we need to know in that case
is whether the standard gate is parameterized or not. If it's not then
we're guaranteed to have a matrix available. This commit updates the
filter logic to account for this and improve it's throughput on standard
gates.

* Use swap_remove instead of shift_remove

* Combine input and output maps into single mapping

This commit combines the `DAGCircuit` `qubit_input_map` and
`qubit_output_map` fields into a single `IndexMap` `qubit_io_map` (and
the same for `clbit_input_map` and `clbit_output_map` going to
`clbit_io_map`). That stores the input and output as 2 element array
where the first element is the input node index and the second element
is the output node index. This reduces the number of lookups we need to
do in practice and also reduces the memory overhead of `DAGCircuit`.

* Ensure we account for clbits in depth() short circuit check

* Also account for Vars in DAGCircuit.width()

The number of vars should be included in the return from the width()
method. This was previously missing in the new implementation of this
method.

* Remove duplicated _get_node() method

The `_get_node()` method was duplicated with the already public
`node()` method. This commit removes the duplicate and updates it's only
usage in the code base.

* Handle Var wires in classical_predecessors

This method was missing the handling for var wires, this commit corrects
the oversight.

* Remove stray comment

* Use Operation::control_flow() instead of isinstance checking

* Use &str for increment_op and decrement_op

This commit reworks the interface for the increment_op and decrement_op
methods to work by reference instead of passing owned String objects to
the methods. Using owned String objects was resulting in unecessary
allocations and extra overhead that could be avoided. There are still a
few places we needed to copy strings to ensure we're not mutating things
while we have references to nodes in the dag, typically only in the
decrement/removal case. But this commit reduces the number of String
copies we need to make in the DAGCircuit.

* Also include vars in depth short circuit

* Fix typing for controlflow name lookup in count_ops

* Fix .properties() method to include operations field

The .properties() method should have included the output of .count_ops()
in its dictionary return but this was commented out temporarily while
other pieces of this PR were fixed. This commit circles back to it and
adds the missing field from the output.

As an aside we should probably deprecate the .properties() method for
removal in 2.0 it doesn't seem to be the most useful method in practice.

* Add missing Var wire handling to py_nodes_on_wire

* Add back optimization to avoid isinstance in op_nodes

This commit adds back an optimization to the op_nodes dag method to
avoid doing a python space op comparison when we're filtering on
non-standard gates and evaluating a standard gate. In these cases we
know that the filter will not match purely from rust without needing a
python space op object creation or an isinstance call so we can avoid
the overhead of doing that.

* Simplify/deduplicate __eq__ method

This commit reworks the logic in the DAGCircuit.__eq__ method
implementation to simplify the code a bit and make it less verbose and
duplicated.

* Invalidate cached py op when needed in substitute_node_with_dag

This commit fixes a potential issue in substitute_node_with_dag() when
the propagate_condition flag was set we were not invalidating cached py
ops when adding a new condition based on a propagated condition. This
could potentially cause the incorrect object to be returned to Python
after calling this method. This fixes the issues by clearing the cached
node so that when returning the op to python we are regenerating the
python object.

* Copy-editing suggestions for release notes

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>

* Fix and simplify separable_circuits()

This commit fixes and simplifies the separable_circuits() method. At
it's core the method is building a subgraph of the original dag for each
weakly connected component in the dag with a little bit of extra
tracking to make sure the graph is a valid DAGCircuit. Instead of trying
to do this manually this commit updates the method implementation to
leverage the tools petgraph gives us for filtering graphs. This both
fixes a bug identified in review but also simplifies the code.

* Add clbit removal test

* Move to using a Vec<[NodeIndex; 2]> for io maps

This commit migrates the qubit_io_map and clbit_io_map to go from a type
of `IndexMap<Qubit, [NodeIndex; 2], RandomState>` to
`Vec<[NodeIndex; 2]>`. Our qubit indices (represented by the `Qubit`
type) must be a contiguous set for the circuit to be valid, and using an
`IndexMap` for these mappings of bit to input and output nodes only
really improved performance in the removal case, but at the cost of
additional runtime overhead for accessing the data. Since removals are
rare and also inefficient because it needs to reindex the entire dag
already we should instead optimize for the accessing the data. Since we
have contiguous indices using a Vec is a natural structure to represent
this mapping.

* Make add_clbits() signature the same as add_qubits()

At some point during the development of this PR the function signatures
between `add_qubits()` and `add_clbits()` diverged between taking a
`Vec<Bound<PyAny>>` and `&Bound<PySequence>`. In general they're are
comprable but since we are going to be working with a `Vec<>` in the
function body this is a better choice to let PyO3 worry about the
conversion for us. Additionally, this is a more natural signature for
rust consumption. This commit just updates `add_clbits()` to use a Vec
too.

* Add attribution comment to num_tensor_factors() method

* Add py argument to add_declared_var()

* Remove unnecessarily Python-space check

* Correct typo in `to_pickle` method

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
raynelfss added a commit to raynelfss/qiskit that referenced this pull request Aug 24, 2024
This commit migrates the entirety of the `DAGCircuit` class to Rust. It
fully replaces the Python version of the class. The primary advantage
of this migration is moving from a Python space rustworkx directed graph
representation to a Rust space petgraph (the upstream library for
rustworkx) directed graph. Moving the graph data structure to rust
enables us to directly interact with the DAG directly from transpiler
passes in Rust in the future. This will enable a significant speed-up in
those transpiler passes. Additionally, this should also improve the
memory footprint as the DAGCircuit no longer stores `DAGNode`
instances, and instead stores a lighter enum NodeType, which simply
contains a `PackedInstruction` or the wire objects directly.

Internally, the new Rust-based `DAGCircuit` uses a `petgraph::StableGraph`
with node weights of type `NodeType` and edge weights of type `Wire`. The
NodeType enum contains variants for `QubitIn`, `QubitOut`, `ClbitIn`,
`ClbitOut`, and `Operation`, which should save us from all of the
`isinstance` checking previously needed when working with `DAGNode` Python
instances. The `Wire` enum contains variants `Qubit`, `Clbit`, and `Var`.

As the full Qiskit data model is not rust-native at this point while
all the class code in the `DAGCircuit` exists in Rust now, there are
still sections that rely on Python or actively run Python code via Rust
to function. These typically involve anything that uses `condition`,
control flow, classical vars, calibrations, bit/register manipulation,
etc. In the future as we either migrate this functionality to Rust or
deprecate and remove it this can be updated in place to avoid the use
of Python.

API access from Python-space remains in terms of `DAGNode` instances to
maintain API compatibility with the Python implementation. However,
internally, we convert to and deal in terms of NodeType. When the user
requests a particular node via lookup or iteration, we inflate an ephemeral
`DAGNode` based on the internal `NodeType` and give them that. This is very
similar to what was done in Qiskit#10827 when porting CircuitData to Rust.

As part of this porting there are a few small differences to keep in
mind with the new Rust implementation of DAGCircuit. The first is that
the topological ordering is slightly different with the new DAGCircuit.
Previously, the Python version of `DAGCircuit` using a lexicographical
topological sort key which was basically `"0,1,0,2"` where the first
`0,1` are qargs on qubit indices `0,1` for nodes and `0,2` are cargs
on clbit indices `0,2`. However, the sort key has now changed to be
`(&[Qubit(0), Qubit(1)], &[Clbit(0), Clbit(2)])` in rust in this case
which for the most part should behave identically, but there are some
edge cases that will appear where the sort order is different. It will
always be a valid topological ordering as the lexicographical key is
used as a tie breaker when generating a topological sort. But if you're
relaying on the exact same sort order there will be differences after
this PR. The second is that a lot of undocumented functionality in the
DAGCircuit which previously worked because of Python's implicit support
for interacting with data structures is no longer functional. For
example, previously the `DAGCircuit.qubits` list could be set directly
(as the circuit visualizers previously did), but this was never
documented as supported (and would corrupt the DAGCircuit). Any
functionality like this we'd have to explicit include in the Rust
implementation and as they were not included in the documented public
API this PR opted to remove the vast majority of this type of
functionality.

The last related thing might require future work to mitigate is that
this PR breaks the linkage between `DAGNode` and the underlying
`DAGCirucit` object. In the Python implementation the `DAGNode` objects
were stored directly in the `DAGCircuit` and when an API method returned
a `DAGNode` from the DAG it was a shared reference to the underlying
object in the `DAGCircuit`. This meant if you mutated the `DAGNode` it
would be reflected in the `DAGCircuit`. This was not always a sound
usage of the API as the `DAGCircuit` was implicitly caching many
attributes of the DAG and you should always be using the `DAGCircuit`
API to mutate any nodes to prevent any corruption of the `DAGCircuit`.
However, now as the underlying data store for nodes in the DAG are
no longer the python space objects returned by `DAGCircuit` methods
mutating a `DAGNode` will not make any change in the underlying
`DAGCircuit`. This can come as quite the surprise at first, especially
if you were relying on this side effect, even if it was unsound.

It's also worth noting that 2 large pieces of functionality from
rustworkx are included in this PR. These are the new files
`rustworkx_core_vnext` and `dot_utils` which are rustworkx's VF2
implementation and its dot file generation. As there was not a rust
interface exposed for this functionality from rustworkx-core there was
no way to use these functions in rustworkx. Until these interfaces
added to rustworkx-core in future releases we'll have to keep these
local copies. The vf2 implementation is in progress in
Qiskit/rustworkx#1235, but `dot_utils` might make sense to keep around
longer term as it is slightly modified from the upstream rustworkx
implementation to directly interface with `DAGCircuit` instead of a
generic graph.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
github-merge-queue bot pushed a commit to Qiskit/qiskit that referenced this pull request Sep 4, 2024
* init

* up

* lint

* .

* up

* before cache

* with cache

* correct

* cleaned up

* lint reno

* Update Cargo.lock

* .

* up

* .

* revert op

* .

* .

* .

* .

* Delete Cargo.lock

* .

* corrected string comparison

* removed Operator class from operation.rs

* .

* Apply suggestions from code review

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

* comments from code review

* Port DAGCircuit to Rust

This commit migrates the entirety of the `DAGCircuit` class to Rust. It
fully replaces the Python version of the class. The primary advantage
of this migration is moving from a Python space rustworkx directed graph
representation to a Rust space petgraph (the upstream library for
rustworkx) directed graph. Moving the graph data structure to rust
enables us to directly interact with the DAG directly from transpiler
passes in Rust in the future. This will enable a significant speed-up in
those transpiler passes. Additionally, this should also improve the
memory footprint as the DAGCircuit no longer stores `DAGNode`
instances, and instead stores a lighter enum NodeType, which simply
contains a `PackedInstruction` or the wire objects directly.

Internally, the new Rust-based `DAGCircuit` uses a `petgraph::StableGraph`
with node weights of type `NodeType` and edge weights of type `Wire`. The
NodeType enum contains variants for `QubitIn`, `QubitOut`, `ClbitIn`,
`ClbitOut`, and `Operation`, which should save us from all of the
`isinstance` checking previously needed when working with `DAGNode` Python
instances. The `Wire` enum contains variants `Qubit`, `Clbit`, and `Var`.

As the full Qiskit data model is not rust-native at this point while
all the class code in the `DAGCircuit` exists in Rust now, there are
still sections that rely on Python or actively run Python code via Rust
to function. These typically involve anything that uses `condition`,
control flow, classical vars, calibrations, bit/register manipulation,
etc. In the future as we either migrate this functionality to Rust or
deprecate and remove it this can be updated in place to avoid the use
of Python.

API access from Python-space remains in terms of `DAGNode` instances to
maintain API compatibility with the Python implementation. However,
internally, we convert to and deal in terms of NodeType. When the user
requests a particular node via lookup or iteration, we inflate an ephemeral
`DAGNode` based on the internal `NodeType` and give them that. This is very
similar to what was done in #10827 when porting CircuitData to Rust.

As part of this porting there are a few small differences to keep in
mind with the new Rust implementation of DAGCircuit. The first is that
the topological ordering is slightly different with the new DAGCircuit.
Previously, the Python version of `DAGCircuit` using a lexicographical
topological sort key which was basically `"0,1,0,2"` where the first
`0,1` are qargs on qubit indices `0,1` for nodes and `0,2` are cargs
on clbit indices `0,2`. However, the sort key has now changed to be
`(&[Qubit(0), Qubit(1)], &[Clbit(0), Clbit(2)])` in rust in this case
which for the most part should behave identically, but there are some
edge cases that will appear where the sort order is different. It will
always be a valid topological ordering as the lexicographical key is
used as a tie breaker when generating a topological sort. But if you're
relaying on the exact same sort order there will be differences after
this PR. The second is that a lot of undocumented functionality in the
DAGCircuit which previously worked because of Python's implicit support
for interacting with data structures is no longer functional. For
example, previously the `DAGCircuit.qubits` list could be set directly
(as the circuit visualizers previously did), but this was never
documented as supported (and would corrupt the DAGCircuit). Any
functionality like this we'd have to explicit include in the Rust
implementation and as they were not included in the documented public
API this PR opted to remove the vast majority of this type of
functionality.

The last related thing might require future work to mitigate is that
this PR breaks the linkage between `DAGNode` and the underlying
`DAGCirucit` object. In the Python implementation the `DAGNode` objects
were stored directly in the `DAGCircuit` and when an API method returned
a `DAGNode` from the DAG it was a shared reference to the underlying
object in the `DAGCircuit`. This meant if you mutated the `DAGNode` it
would be reflected in the `DAGCircuit`. This was not always a sound
usage of the API as the `DAGCircuit` was implicitly caching many
attributes of the DAG and you should always be using the `DAGCircuit`
API to mutate any nodes to prevent any corruption of the `DAGCircuit`.
However, now as the underlying data store for nodes in the DAG are
no longer the python space objects returned by `DAGCircuit` methods
mutating a `DAGNode` will not make any change in the underlying
`DAGCircuit`. This can come as quite the surprise at first, especially
if you were relying on this side effect, even if it was unsound.

It's also worth noting that 2 large pieces of functionality from
rustworkx are included in this PR. These are the new files
`rustworkx_core_vnext` and `dot_utils` which are rustworkx's VF2
implementation and its dot file generation. As there was not a rust
interface exposed for this functionality from rustworkx-core there was
no way to use these functions in rustworkx. Until these interfaces
added to rustworkx-core in future releases we'll have to keep these
local copies. The vf2 implementation is in progress in
Qiskit/rustworkx#1235, but `dot_utils` might make sense to keep around
longer term as it is slightly modified from the upstream rustworkx
implementation to directly interface with `DAGCircuit` instead of a
generic graph.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* Update visual mpl circuit drawer references

Right now there is a bug in the matplotlib circuit visualizer likely
caused by the new `__eq__` implementation for `DAGOpNode` that didn't
exist before were some gates are missing from the visualization. In the
interest of unblocking this PR this commit updates the references for
these cases temporarily until this issue is fixed.

* Ensure DAGNode.sort_key is always a string

Previously the sort_key attribute of the Python space DAGCircuit was
incorrectly being set to `None` for rust generated node objects. This
was done as for the default path the sort key is determined from the
rust domain's representation of qubits and there is no analogous data in
the Python object. However, this was indavertandly a breaking API change
as sort_key is expected to always be a string. This commit adds a
default string to use for all node types so that we always have a
reasonable value that matches the typing of the class. A future step is
likely to add back the `dag` kwarg to the node types and generate the
string on the fly from the rust space data.

* Make Python argument first in Param::eq and Param::is_close

The standard function signature convention for functions that take a
`py: Python` argument is to make the Python argument the first (or
second after `&self`). The `Param::eq` and `Param::is_close` methods
were not following this convention and had `py` as a later argument in
the signature. This commit corrects the oversight.

* Fix merge conflict with #12943

With the recent merge with main we pulled in #12943 which conflicted
with the rust space API changes made in this PR branch. This commit
updates the usage to conform with the new interface introduced in this
PR.

* Add release notes and test for invalid args on apply methods

This commit adds several release notes to document this change. This
includes a feature note to describe the high level change and the user
facing benefit (mainly reduced memory consumption for DAGCircuits),
two upgrade notes to document the differences with shared references
caused by the new data structure, and a fix note documenting the fix
for how qargs and cargs are handled on `.apply_operation_back()` and
`.apply_operation_front()`. Along with the fix note a new unit test is
added to serve as a regression test so that we don't accidentally allow
adding cargs as qargs and vice versa in the future.

* Restore `inplace` argument functionality for substitute_node()

This commit restores the functionality of the `inplace` argument for
`substitute_node()` and restores the tests validating the object
identity when using the flag. This flag was originally excluded from
the implementation because the Rust representation of the dag is not
a shared reference with Python space and the flag doesn't really mean
the same thing as there is always a second copy of the data for Python
space now. The implementation here is cheating slighty as we're passed
in the DAG node by reference it relies on that reference to update the
input node at the same time we update the dag. Unlike the previous
Python implementation where we were updating the node in place and the
`inplace` argument was slightly faster because everything was done by
reference. The rust space data is still a compressed copy of the data
we return to Python so the `inplace` flag will be slightly more
inefficient as we need to copy to update the Python space representation
in addition to the rust version.

* Revert needless dict() cast on metadata in dag_to_circuit()

This commit removes an unecessary `dict()` cast on the `dag.metadata`
when setting it on `QuantumCircuit.metadata` in
`qiskit.converters.dag_to_circuit()`. This slipped in at some point
during the development of this PR and it's not clear why, but it isn't
needed so this removes it.

* Add code comment for DAGOpNode.__eq__ parameter checking

This commit adds a small inline code comment to make it clear why we
skip parameter comparisons in DAGOpNode.__eq__ for python ops. It might
not be clear why the value is hard coded to `true` in this case, as this
check is done via Python so we don't need to duplicate it in rust space.

* Raise a ValueError on DAGNode creation with invalid index

This commit adds error checking to the DAGNode constructor to raise a
PyValueError if the input index is not valid (any index < -1).
Previously this would have panicked instead of raising a user catchable
error.

* Use macro argument to set python getter/setter name

This commit updates the function names for `get__node_id` and
`set__node_id` method to use a name that clippy is happy with and
leverage the pyo3 macros to set the python space name correctly instead
of using the implicit naming rules.

* Remove Ord and PartialOrd derives from interner::Index

The Ord and PartialOrd traits were originally added to the Index struct
so they could be used for the sort key in lexicographical topological
sorting. However, that approach was abandonded during the development of
this PR and instead the expanded Qubit and Clbit indices were used
instead. This left the ordering traits as unnecessary on Index and
potentially misleading. This commit just opts to remove them as they're
not needed anymore.

* Fix missing nodes in matplotlib drawer.

Previously, the change in equality for DAGNodes was causing nodes
to clobber eachother in the matplotlib drawer's tracking data
structures when used as keys to maps.

To fix this, we ensure that all nodes have a unique ID across
layers before constructing the matplotlib drawer. They actually
of course _do_ in the original DAG, but we don't really care
what the original IDs are, so we just make them up.

Writing to _node_id on a DAGNode may seem odd, but it exists
in the old Python API (prior to being ported to Rust) and
doesn't actually mutate the DAG at all since DAGNodes are
ephemeral.

* Revert "Update visual mpl circuit drawer references"

With the previous commit the bug in the matplotlib drawer causing the
images to diverge should be fixed. This commit reverts the change to the
reference images as there should be no difference now.

This reverts commit 1e4e6f3.

* Update visual mpl circuit drawer references for control flow circuits

The earlier commit that "fixed" the drawers corrected the visualization
to match expectations in most cases. However after restoring the
references to what's on main several comparison tests with control flow
in the circuit were still failing. The failure mode looks similar to the
other cases, but across control flow blocks instead of at the circuit
level. This commit temporarily updates the references of these to the
state of what is generated currently to unblock CI. If/when we have a
fix this commit can be reverted.

* Apply suggestions from code review

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

* code review

* Fix edge cases in DAGOpNode.__eq__

This commit fixes a couple of edge cases in DAGOpNode.__eq__ method
around the python interaction for the method. The first is that in
the case where we had python object parameter types for the gates we
weren't comparing them at all. This is fixed so we use python object
equality for the params in this case. Then we were dropping the error
handling in the case of using python for equality, this fixes it to
return the error to users if the equality check fails. Finally a comment
is added to explain the expected use case for `DAGOpNode.__eq__` and why
parameter checking is more strict than elsewhere.

* Remove Param::add() for global phase addition

This commit removes the Param::add() method and instead adds a local
private function to the `dag_circuit` module for doing global phase
addition. Previously the `Param::add()` method was used solely for
adding global phase in `DAGCircuit` and it took some shortcuts knowing
that context. This made the method implementation ill suited as a
general implementation.

* More complete fix for matplotlib drawer.

* Revert "Update visual mpl circuit drawer references for control flow circuits"

This reverts commit 9a6f953.

* Unify rayon versions in workspace

* Remove unused _GLOBAL_NID.

* Use global monotonic ID counter for ids in drawer

The fundamental issue with matplotlib visualizations of control flow is
that locally in the control flow block the nodes look the same but are
stored in an outer circuit dictionary. If the gates are the same and on
the same qubits and happen to have the same node id inside the different
control flow blocks the drawer would think it's already drawn the node
and skip it incorrectly. The previous fix for this didn't go far enough
because it wasn't accounting for the recursive execution of the drawer
for inner blocks (it also didn't account for LayerSpoolers of the same
length).

* Remove unused BitData iterator stuff.

* Fully port Optimize1qGatesDecomposition to Rust

This commit builds off of #12550 and the other data model in Rust
infrastructure and migrates the Optimize1qGatesDecomposition pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly calibrations and parameter
expressions (for global phase). But otherwise the entirety of the pass
operates in rust now.

This is just a first pass at the migration here, it moves the pass to be
a single for loop in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done by the pass so we should be able to the
throughput of the pass by leveraging multithreading to handle each run
in parallel. This commit does not attempt this though, because of the
Python dependency and also the data structures around gates and the
dag aren't really setup for multithreading yet and there likely will
need to be some work to support that (this pass is a good candidate to
work through the bugs on that).

Part of #12208

* remove with_gil in favor of passing python tokens as params

* Apply suggestions from code review

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

* fmt

* python serialization

* deprecation

* Update commutation_checker.py

* heh

* init

* let Pytuple collect

* lint

* First set of comments

- use Qubit/Clbit
- more info on unsafe
- update reno
- use LazySet less
- use OperationRef, avoid CircuitInstruction creation

* Second part

- clippy
- no BigInt
- more comments

* Matrix speed & fix string sort

-- could not use op.name() directly since sorted differently than Python, hence it's back to BigInt

* have the Python implementation use Rust

* lint & tools

* remove unsafe blocks

* One more try to avoid segfaulty windows

-- if that doesn't work maybe revert the change the the Py CommChecker uses Rust

* Original version

Co-authored-by: Sebastian Brandhofer <148463728+sbrandhsn@users.noreply.github.com>

* Sync with updated CommutationChecker

todo: shouldn't make the qubits interner public

* Debug: disable cache

trying to figure out why the windows CI fails (after being unable to locally reproduce we're using CI with a reduced set of tests)

* ... second try

* Update crates/accelerate/src/commutation_checker.rs

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

* Restore azure config

* Remove unused import

* Revert "Debug: disable cache"

This reverts commit c564b80.

* Don't overallocate cache

We were allocating a the cache hashmap with a capacity for max cache
size entries every time we instantiated a new CommutationChecker. The
max cache size is 1 million. This meant we were allocating 162MB
everytime CommutationChecker.__new__ was called, which includes each
time we instantiate it manually (which happens once on import), the
CommutationAnalysis pass gets instantiated (twice per preset pass
manager created with level 2 or 3), or a commutation checker instance is
pickle deserialized. This ends up causing a fairly large memory
regression and is the source of the CI failures on windows.

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* Cleanup parameter key type to handle edge conditions better

This commit cleans up the ParameterKey type and usage to make it handle
edge conditions better. The first is that the type just doesn't do the
right thing for NaN, -0, or the infinities. Canonicalization is added
for hash on -0 and the only constructor of the newtype adds a runtime
guard against NaN and inifinity (positive or negative) to avoid that
issue. The approach only makes sense as the cache is really there to
guard us against unnecessary re-computing when we reiterate over the
circuit > 1 time and nothing has changed for gates. Otherwise comparing
floats like done in this PR does would not be a sound or an effective
approach.

* Remove unnecessary cache hit rate tracking

* Undo test assertion changes

* Undo unrelated test changes

* Undo pending deprecation and unify commutation classes

This commit removes the pending deprecation decorator from the python
class definition as the Python class just internally is using the rust
implementation now. This also removes directly using the rust
implementation for the standard commutation library global as using the
python class is exactly the same now.

We can revisit if there is anything we want to deprecate and remove in
2.0 in a follow up PR. Personally, I think the cache management methods
are all we really want to remove as the cache should be an internal
implementation detail and not part of the public interface.

* Undo gha config changes

* Make serialization explicit

This commit makes the pickling of cache entries explicit. Previously it
was relying on conversion traits which hid some of the complexity but
this uses a pair of conversion functions instead.

* Remove stray SAFETY comment

* Remove ddt usage from the tests

Now that the python commutation checker and the rust commutation checker
are the same thing the ddt parameterization of the commutation checker
tests was unecessary duplication. This commit removes the ddt usage to
restore having a single run of all the tests.

* Update release note

* Fix CommutationChecker class import

* Remove invalid test assertion for no longer public attribute

* Ray's review comments

Co-authored-by: Raynel Sanchez <raynelfss@hotmail.com>

* Handle ``atol/rtol``, more error propagation

* update to latest changes in commchecker

* fix merge conflict remnants

* re-use expensive quantities

such as the relative placement and the parameter hash

* add missing header

* gentler error handling

* review comments & more docs

* Use vec over IndexSet + clippy

- vec<vec> is slightly faster than vec<indexset>
- add custom types to satisfies clippy's complex type complaint
- don't handle Clbit/Var

* Simplify python class construction

Since this PR was first written the split between the python side and
rust side of the CommutationChecker class has changed so that there are
no longer separate classes anymore. The implementations are unified and
the python space class just wraps an inner rust object. However, the
construction of the CommutationAnalysis pass was still written assuming
there was the possibility to get either a rust or Python object. This
commit fixes this and the type change on the `comm_checker` attribute by
removing the unnecessary logic.

---------

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
Co-authored-by: Kevin Hartman <kevin@hart.mn>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
Co-authored-by: Raynel Sanchez <raynelfss@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose vf2_mapping like interface to rustworkx-core
4 participants