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

Integrating BQSKit into MQTBench benchmark generation module #286

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

jagandecapri
Copy link
Contributor

This PR addresses #282. In summary, a new module called bsqkit_helper is created to integrate BSQKit into MQTBench. There are some open issues. Namely, QASM generation of the benchmark is not working at the moment and the test cases currently doesn’t test for QASM generation. There are also other bugs as highlighted below. Numpy was also used in the module which is open for discussion as I saw Numpy is not a dependency in MQTBench currently. Test coverage might be lower as I didn’t add new test cases for the compiler cache and custom gates (ECR and XXPlusYY). Transpilation to only QASM 2.0 gates using Qiskit is needed before converting to bqskit circuit as not all Qiskit circuit gates that is used in the benchmark has an equivalent in bqskit, e.g: mcx gate. Without the transpilation, bsqkit converts the unsupported gate to arbitrarily large block of custom gate which then fails to compile. Runtime for test cases seems to have increased quite considerably with the introduction of bqskit. More details as below:

Bugs in bqskit

QASM string representation

cry gate bug

  • Issue Compilation seems to be stuck with multiple cry gates BQSKit/bqskit#217. Due to this, native and mapped level test cases which has cry gate(s) execute as following:
    • pricingcall test case executes indefinitely
    • pricingput and random test case sometimes works and sometimes face the same problem of indefinite execution as pricingcall. For random, since it is a random circuit from Qiskit, I suspect when there are multiple cry gates, the test cases starts to go into indefinite execution

New improvements

Custom gates

  • Implemented two custom gates, ECR and XXPlusYY, following Qiskit little-endian convention. The former is supported natively by OQC Lucy and the latter by Rigetti.
    • ECR Gate
      • Type check is disabled for inherited bqskit classes as mypy throws error otherwise. It checks for type information from the inherited classes which doesn’t support type checks I think. Not sure whether this is the right fix.
      • Need verification whether unitary implemented is correct. Obtained it from Qiskit documentation for ECR Gate
      • Created issue in bqskit to add this gate in their library Proposal to include echoed-resonance (ECR) gate into BQSkit. BQSKit/bqskit#216
    • XXPlusYY Gate
      • Largely inspired by bqskit FSIM Gate
      • Type check is disabled for inherited bqskit classes as mypy throws error otherwise. It checks for type information from the inherited classes which doesn’t support type checks I think. Not sure whether this is the right fix.
      • There are two types of optimization algorithm that is used by bqskit for parameterized gates. They are DifferentiableUnitary and LocallyOptimizableUnitary. A DifferentiableUnitary can be used in instantiation calls that use a gradient based optimization subroutine. On the other hand,LocallyOptimizableUnitarycan be used in instantiation calls that use a tensor-network based approach like QFactor. More info at here
      • The gate implements only the function for differentiable unitary optimization. From my understanding reading the FSIM and U3 gate implementation, differentiable unitary returns a numpy ndarray containing the partial differentation of the gate’s unitary matrix with respect to each parameter which is what I have implemented. On the other hand, for locally optimizable unitary, I’m not really sure what is the right implementation.
      • Need verification whether unitary in get_unitary and get_grad is correct. Obtained unitary from Qiskit documentation for XXPlusYY gate. As for the gradient, I derived manually and verified with WolframAlpha.
      • This gate uses numpy library in the get_grad function. However, numpy is not listed as dependency in pyproject.toml. I think it gets installed with other library. Should we make it a dependency in MQTBench? if we agree on using numpy, then, some functionality that is using cmath and math module in bsqkit_helper can be replaced with numpy
      • Issue could be created in bsqkit if they are interested to add this gate to their library

CPhase gate

  • Added CPhase gate for Rigetti native gates

Cache for compiler

  • Each call to the compile function creates resources to parallelize the compilation across the processors in the system. Hence, there is a waiting time for the setup to occur. Bqskit documentation suggested caching the compiler instance for repeated call of the compile function which is implemented in the CacheCompiler class using the singleton design pattern
  • The compiler instance is registed with the atexit module to close it when the script exits
  • Type check is suppressed for __new__ method as ruff was throwing error for the return type which seems to be correct from my point of view

Runtime of test case

  • I timed the pytest command time pytest -k 'test_quantumcircuit_native_and_mapped_levels' for only bqskit, minus the circuits mentioned in the cry bug section above. It takes ~54 minutes to run this one test function in my laptop. The other test functions execution time seems reasonable

Code structure

The bsqkit_helper class seems to be doing a lot of things, managing cache for the compiler, creating custom gates and benchmark generation. Maybe it could be broken to different files. I didn’t want to change the structure of the repository, so I’ve put all of it in the helper class. Open for discussion.

With that, looking forward to your kind review and discussions. 🙂

In independent level benchmark, circuit is transpiled to QASM 2.0
compatible. This is because there are some gates used in Qiskit such as
`mcx` and `mcphase` which are not correctly decomposed and converted by
bqskit.
Error threshold is used for compilation verification. As stated
in bqskit tutorial, verification is not for free. It is exponentially
costly in time. By default, bqskit doesn't do verification. For
MQTBench, verification may not be necessary.
target_directory: str = "./",
target_filename: str = "",
) -> Circuit:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
target_directory: str = "./",
target_filename: str = "",
) -> bool:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
target_directory: str = "./",
target_filename: str = "",
) -> Circuit:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
target_directory: str = "./",
target_filename: str = "",
) -> bool:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
target_directory: str = "./",
target_filename: str = "",
) -> Circuit:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
target_directory: str = "./",
target_filename: str = "",
) -> bool:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
Bqskit code in benchmark generator was not properly setup.
Test cases were also not properly setup. This commit fixes both.
Note that the test convert bqskit circuit to qiskit is commented out
because of the bug in bqskit BQSKit/bqskit#214
The test case fix is just to get it passing. It doesn't test
the actual QASM file generation in the functions. I have left
TODOs in the code to remind me to add these tests later.
TODOs are added to test_bench to indicate the test case code that
needs to be updated once the bugs in bqskit library are fixed.
@jagandecapri
Copy link
Contributor Author

Fixed failing test cases due to bugs in code. Some test cases were left to failed intentionally to indicate the part that needs to be updated when the bugs in bqskit library are fixed. Alternatively, in some cases, I have left TODOs comments in test_bench.py to indicate the parts that needs updating once the bugs in bqskit library are fixed.

Copy link
Collaborator

@nquetschlich nquetschlich left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your effort and this PR, @jagandecapri. I am very happy that we will (soon) also support BQSKit as another compiler. I have just mentioned a few things that caught my attention. Additionally I was wondering, whether we really need all the newly introduced type ignores. Please carefully double check, whether those are actually necessary.

@@ -19,6 +19,7 @@ requires-python = ">=3.9"
dynamic = ["version"]

dependencies = [
"bqskit>=1.1.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to add an upper cap as well.

@@ -141,6 +148,31 @@ def generate_mapped_levels(
) -> None:
for provider in get_available_providers():
for device in provider.get_available_devices():
for opt_level in [1, 2, 3, 4]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be iterate over BQSKitSettings that cover those optimization levels? I am aware that this is not the case for Qiskit and TKET as well but desired (see #88). Therefore, I would like this new implementation to already follow this direction.

@@ -198,6 +230,28 @@ def generate_native_gates_levels(
parameter_space: list[tuple[int, str]] | list[int] | list[str] | range,
) -> None:
for provider in get_available_providers():
for opt_level in [1, 2, 3, 4]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

See above.

from mqt.bench.devices import Device, Provider


class CachedCompiler:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed? Do we really need to have this definition in our code and cannot use something from BQSKit?

"cx": CNOTGate(),
"cz": CZGate(),
"ecr": ECRGate(),
"xx_plus_yy": XXPlusYYGate(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you also make sure that this gate is implemented for TKET as well in the tket_helper.py file? Same for the cp gate.

except Exception as e:
print("BQSKit Exception Indep: ", e)
return False

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is the BQSKit compilation happening? I don't see it here.

print("BQSKit Exception NativeGates: ", e)
return False

gate_set = provider.get_native_gates()
Copy link
Collaborator

Choose a reason for hiding this comment

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

all those compilation commands should be within the try block

@@ -70,15 +73,18 @@ def sample_filenames() -> list[str]:
"ae_indep_qiskit_10.qasm",
"ghz_nativegates_rigetti_qiskit_opt3_54.qasm",
"ae_indep_tket_93.qasm",
"ae_indep_bqskit_93.qasm",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a smaller circuit would be better suited for BQSKit because of their runtime (especially for optimization levels >2)

("indep", 2),
("mapped", 9),
("indep", 3),
("mapped", 11),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have the test case sizes changed?

@jagandecapri
Copy link
Contributor Author

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.

2 participants