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

Fixes for wire management in Catalyst #784

Merged
merged 8 commits into from
Jun 7, 2024
Merged

Fixes for wire management in Catalyst #784

merged 8 commits into from
Jun 7, 2024

Conversation

dime10
Copy link
Contributor

@dime10 dime10 commented Jun 3, 2024

This PR contains several improvements regarding the wire handling in Catalyst:

  • the runtime will now raise an exception when a qubit is accessed out of bounds, which improves memory safety, and doesn't require the backend to raise an invalid qubit error
  • the arbitrary restriction of requiring at least one qubit to run a quantum circuit is lifted (now follows PL behaviour)
  • the QJITDevice classes in the frontend now perform stricter validation of existing assumptions on the wires property of devices

Related GitHub Issues: #780

[sc-65105]

@dime10 dime10 added bug Something isn't working runtime Pull requests that update the runtime compiler Pull requests that update the compiler frontend Pull requests that update the frontend labels Jun 3, 2024
@dime10 dime10 requested a review from erick-xanadu June 3, 2024 23:44
No new assumptions are made, but implicit assumptions are made more
explicit.
Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.09%. Comparing base (ec9c826) to head (664d31b).
Report is 188 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #784   +/-   ##
=======================================
  Coverage   98.08%   98.09%           
=======================================
  Files          70       70           
  Lines        9731     9740    +9     
  Branches      780      782    +2     
=======================================
+ Hits         9545     9554    +9     
  Misses        151      151           
  Partials       35       35           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WrathfulSpatula
Copy link
Contributor

@dime10 This does, in fact, allow allocating 0 qubits (at least trivally) and provides a helpful error message when wire count is too low the requested circuit.

Thank you! LGTM! (With that said, the issue about suspicious "extra" phase gates in tutorial_qft.py remains, as per the issue.)

@dime10
Copy link
Contributor Author

dime10 commented Jun 4, 2024

@dime10 This does, in fact, allow allocating 0 qubits (at least trivally) and provides a helpful error message when wire count is too low the requested circuit.

Thank you! LGTM! (With that said, the issue about suspicious "extra" phase gates in tutorial_qft.py remains, as per the issue.)

Thank you! I will have another look at the original issue then :)

Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

Do you know why it fails on kokkos?

@dime10
Copy link
Contributor Author

dime10 commented Jun 5, 2024

Do you know why it fails on kokkos?

Hmm, will need to investigate :/

frontend/catalyst/device/qjit_device.py Outdated Show resolved Hide resolved
frontend/catalyst/device/qjit_device.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_device_api.py Outdated Show resolved Hide resolved
@erick-xanadu
Copy link
Contributor

@dime10 it is an issue with kokkos:

>>> dev = qml.device("lightning.kokkos", 0)
Kokkos::OpenMP::initialize WARNING: OMP_PROC_BIND environment variable not set
  In general, for best performance with OpenMP 4.0 or better set OMP_PROC_BIND=spread and OMP_PLACES=threads
  For best performance with OpenMP 3.1 set OMP_PROC_BIND=true
  For unit testing set OMP_PROC_BIND=false

>>> qml.qnode(dev)(lambda : qml.state())()
Segmentation fault (core dumped)

@dime10
Copy link
Contributor Author

dime10 commented Jun 5, 2024

@dime10 it is an issue with kokkos:

>>> dev = qml.device("lightning.kokkos", 0)
Kokkos::OpenMP::initialize WARNING: OMP_PROC_BIND environment variable not set
  In general, for best performance with OpenMP 4.0 or better set OMP_PROC_BIND=spread and OMP_PLACES=threads
  For best performance with OpenMP 3.1 set OMP_PROC_BIND=true
  For unit testing set OMP_PROC_BIND=false

>>> qml.qnode(dev)(lambda : qml.state())()
Segmentation fault (core dumped)

And it only happens with 0 qubits. Could be an upstream issue then, while default.qubit and lightning.qubit work fine in that regime, lightning.kokkos might not.

@dime10
Copy link
Contributor Author

dime10 commented Jun 5, 2024

@erick-xanadu I've disabled those tests on kokkos, don't think it's particularly important that they run on all devices. It's mainly to see that Catalyst allows 0 qubits end to end.

Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

Just that minor comment.

@dime10 dime10 merged commit f1f1555 into main Jun 7, 2024
36 of 37 checks passed
@dime10 dime10 deleted the wire-fixes branch June 7, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler Pull requests that update the compiler frontend Pull requests that update the frontend runtime Pull requests that update the runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants