Skip to content

Commit

Permalink
Oxidize commutation checker (#12870)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>

* comments from code review

* Apply suggestions from code review

Co-authored-by: Raynel Sanchez <[email protected]>

* code review

* remove with_gil in favor of passing python tokens as params

* Apply suggestions from code review

Co-authored-by: Raynel Sanchez <[email protected]>

* fmt

* python serialization

* deprecation

* Update commutation_checker.py

* heh

* 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

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

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

* re-use expensive quantities

such as the relative placement and the parameter hash

---------

Co-authored-by: Raynel Sanchez <[email protected]>
Co-authored-by: Matthew Treinish <[email protected]>
Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: Jake Lishman <[email protected]>
Co-authored-by: Raynel Sanchez <[email protected]>
  • Loading branch information
6 people authored Sep 3, 2024
1 parent b501db1 commit df0ad77
Show file tree
Hide file tree
Showing 12 changed files with 1,287 additions and 609 deletions.
74 changes: 71 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/accelerate/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ faer = "0.19.2"
itertools.workspace = true
qiskit-circuit.workspace = true
thiserror.workspace = true
ndarray_einsum_beta = "0.7"
once_cell = "1.19.0"

[dependencies.smallvec]
workspace = true
Expand Down
Loading

0 comments on commit df0ad77

Please sign in to comment.