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

Add find_successors_by_edge #319

Merged
merged 6 commits into from
May 21, 2021
Merged

Conversation

IvanIsCoding
Copy link
Collaborator

Related to #265

This PR adds two new functions to PyDiGraph: find_sucessors_by_edge and find_predecessors_by_edge. These functions can be useful for speeding up routines that want a subset of successors/predecessors that match a filter function.

With these new functions, it is possible to find the successors/predecessors of an node that match a filter in O(e'), where e' is the number of edges connected to the node. Before, the simplest way to achieve the same behavior was to get all edges for each successor/predecessor and then filter, which was O(d * e'), where d is the number of distinct neighbors a node has.

These new functions will be immediatelly applicable to the quantum_successors and quantum_precessors methods in qiskit-terra DAGCircuit. They will consequently provide a speed up to collect 2q blocks, which has quantum_successors and quantum_precessors as its most expensive steps during graph traversal.

@coveralls
Copy link

coveralls commented May 18, 2021

Pull Request Test Coverage Report for Build 864490740

  • 55 of 56 (98.21%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 96.8%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/digraph.rs 55 56 98.21%
Totals Coverage Status
Change from base Build 864388546: 0.02%
Covered Lines: 7714
Relevant Lines: 7969

💛 - Coveralls

Copy link
Contributor

@nahumsa nahumsa left a comment

Choose a reason for hiding this comment

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

LGTM. Very clean implementation and tests.
Just one question: have you benchmarked this improvement comparing with the previous implementation?

@IvanIsCoding
Copy link
Collaborator Author

IvanIsCoding commented May 18, 2021

LGTM. Very clean implementation and tests.
Just one question: have you benchmarked this improvement comparing with the previous implementation?

I benchmarked the function with the code below and I got a 2x improvement:

import retworkx
import timeit

dag = retworkx.PyDAG()
node_a = dag.add_node(0)
node_b = dag.add_child(node_a, 1, {"a": 1})
node_c = dag.add_child(node_b, 2, {"a": 2})
dag.add_child(node_c, 3, {"a": 1})



def old_method(dag, node):
	result = []

	for succ in dag.successors(node):
		if any(x["a"] % 2 == 0 for x in dag.get_all_edge_data(node, succ)):
			result.append(succ)

	return result

print(
	"Old Code without optmization: ",
	timeit.timeit(
		'old_method(dag, node_b)', 
		globals=globals(), 
	)
)

print(
	"New Code with optmization: ",
	timeit.timeit(
		'dag.find_successors_by_edge(node_b, lambda x: x["a"] % 2 == 0)', 
		globals=globals(), 
	)
)

Old Code without optmization: 0.8768505390034989
New Code with optmization: 0.3845832150109345

The improvement becomes larger as the number of distinct neighbors increases. If the node has 10 distinct neighbors, then you get a 10x improvement. For qiskit-terra, on average there are 2 neighbors so that is why I claim a 2x improvement.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this

@mtreinish mtreinish merged commit 6802761 into Qiskit:main May 21, 2021
@IvanIsCoding IvanIsCoding deleted the filtered_successors branch July 25, 2021 04:25
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.

4 participants