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

Replace NetworkX by RetworkX #1791

Merged
merged 32 commits into from
Dec 22, 2021
Merged

Replace NetworkX by RetworkX #1791

merged 32 commits into from
Dec 22, 2021

Conversation

maliasadi
Copy link
Member

@maliasadi maliasadi commented Oct 22, 2021

Context:
In CircuitGraph, replace NetworkX (NX) by RetworkX (RX) and add RX support to QAOA in PennyLane.

Description of the Change:
RX is written in Rust providing a general purpose graph library. This library pursues to offer functionalities similar to what NX offers but with much faster performance. Pennylane uses NX to implement circuit graphs and QAOA with many use cases in the entire library. This PR replaces NX by RX and investigates the performance of RX in Pennylane.

Benefits:
Using RX instead of NX, PennyLane can take advantage of

  • The performance and safety that Rust provides; and
  • Faster (un)directed graph operations for CircuitGraph and QAOA, and their applications.

Benchmarks:
Check here

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@josh146
Copy link
Member

josh146 commented Dec 16, 2021

@maliasadi can I ask the status of this PR? Is it mainly a prototype that can be closed, or is it still WIP?

@maliasadi
Copy link
Member Author

@maliasadi can I ask the status of this PR? Is it mainly a prototype that can be closed, or is it still WIP?

Hey @josh146. To replace all the use cases of NetworkX with RetworkX, we need some functionalities that is planned to be added to the next release of RetworkX. So, we were waiting for the next release indeed. We can close the PR and open it again when we have every pieces of the puzzle.

@maliasadi maliasadi closed this Dec 16, 2021
@maliasadi maliasadi reopened this Dec 17, 2021
@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #1791 (af47884) into master (ebebb5e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1791   +/-   ##
=======================================
  Coverage   99.17%   99.17%           
=======================================
  Files         225      225           
  Lines       17204    17284   +80     
=======================================
+ Hits        17062    17142   +80     
  Misses        142      142           
Impacted Files Coverage Δ
pennylane/circuit_graph.py 95.00% <100.00%> (+0.98%) ⬆️
pennylane/qaoa/cost.py 100.00% <100.00%> (ø)
pennylane/qaoa/cycle.py 98.88% <100.00%> (-1.12%) ⬇️
pennylane/qaoa/mixers.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebebb5e...af47884. Read the comment docs.

@maliasadi maliasadi marked this pull request as ready for review December 20, 2021 02:00
@maliasadi maliasadi changed the title [WIP] Replace networkx by retworkx in Pennylane Replace NetworkX by RetworkX in Pennylane Dec 20, 2021
@maliasadi maliasadi changed the title Replace NetworkX by RetworkX in Pennylane Replace NetworkX by RetworkX Dec 20, 2021
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Wonderful work @maliasadi!

Very happy to approve conditional on updating the changelog to add an entry on this change.

Do you have any benchmark plots on using PL before/after this change?

pennylane/circuit_graph.py Show resolved Hide resolved
pennylane/circuit_graph.py Show resolved Hide resolved
pennylane/circuit_graph.py Outdated Show resolved Hide resolved
pennylane/circuit_graph.py Show resolved Hide resolved
pennylane/circuit_graph.py Outdated Show resolved Hide resolved
pennylane/qaoa/cost.py Show resolved Hide resolved
pennylane/qaoa/cycle.py Outdated Show resolved Hide resolved
pennylane/qaoa/cycle.py Outdated Show resolved Hide resolved
pennylane/qaoa/cycle.py Outdated Show resolved Hide resolved
pennylane/qaoa/cycle.py Outdated Show resolved Hide resolved
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Heroic effort @maliasadi ! Great work on this.
I agree with @josh146's comments, and added some small rec's in there.

@@ -18,7 +18,7 @@
# pylint: disable=too-many-branches,too-many-arguments,too-many-instance-attributes
from collections import Counter, OrderedDict, namedtuple

import networkx as nx
import retworkx as rx
Copy link
Member

Choose a reason for hiding this comment

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

💯

pennylane/qaoa/cycle.py Outdated Show resolved Hide resolved
pennylane/qaoa/cycle.py Outdated Show resolved Hide resolved
pennylane/qaoa/cycle.py Outdated Show resolved Hide resolved
pennylane/qaoa/cycle.py Outdated Show resolved Hide resolved
@maliasadi
Copy link
Member Author

Thank you @josh146 for the great suggestions!

Do you have any benchmark plots on using PL before/after this change?

Currently, I have benchmarks comparing NX vs. RX methods used in PL:
Create
Compute

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Thanks @maliasadi, happy to approve! 💪

Currently, I have benchmarks comparing NX vs. RX methods used in PL

These benchmarks look great 😍

I have one request: do you think you would be able to quickly benchmark a more user-facing workflow? For example:

  • the resulting speedup for computing cost and mixer hamiltonians in the QAOA module

  • resulting speedup when computing something like the metric_tensor, for a circuit with a lot of layers (internally, the metric tensor computes a DAG in order to identify the 'layers' of a circuit).

More user-facing benchmarks would be super helpful, as it allows us to advertise the practical effects of this change next release 🙂

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/circuit_graph.py Show resolved Hide resolved
pennylane/circuit_graph.py Show resolved Hide resolved
pennylane/circuit_graph.py Outdated Show resolved Hide resolved
pennylane/circuit_graph.py Outdated Show resolved Hide resolved
Co-authored-by: Josh Izaac <josh146@gmail.com>
@maliasadi
Copy link
Member Author

  • resulting speedup when computing something like the metric_tensor, for a circuit with a lot of layers (internally, the metric tensor computes a DAG in order to identify the 'layers' of a circuit).

I collected results for test cases in transforms/test_metric_tensor.py on a machine with Intel Core i7-1185G7 @ 3.00GHz:
metric_tensor

Regarding QAOA routines, they are not using any advanced graph routines, and so, there shouldn't be significant speedup by using RX over NX. However, building graphs in RX is much faster and optimizer than NX (check here for benchmarks) and so there are big improvements on the execution-time of QAOA examples. In ./tests/test_qaoa.py, there were 79 tests and it took ~4.15s to run, now we test 129 tests (including RX test cases) and takes ~4.33s.

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Comments look good to me @maliasadi! And thanks for the additional benchmarking :)

pennylane/circuit_graph.py Show resolved Hide resolved
@maliasadi maliasadi merged commit 20f1dd8 into master Dec 22, 2021
@maliasadi maliasadi deleted the add_retworkx_support branch December 22, 2021 06:50
@josh146 josh146 mentioned this pull request Feb 4, 2022
11 tasks
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.

3 participants