Adding Transitive Reduction Function #923

danielleodigie · 2023-07-06T14:23:28Z

Aims to fix #801
Adds transitive_reduction function to DAG algorithms module

…ted tests for filter_edges and filter_nodes for both PyGraph and PyDiGraph. Created release notes for the functions.

…matrix

elease notes

… Wrote tests.

…rustworkx into adjacency-matrix

…gie/rustworkx into transitive-reduction

coveralls · 2023-07-06T14:41:54Z

Pull Request Test Coverage Report for Build 5811368748

65 of 65 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 96.474%

Totals
Change from base Build 5787658451:	0.01%
Covered Lines:	15378
Relevant Lines:	15940

💛 - Coveralls

raynelfss

Everything here looks great to me. I left a couple comments that are up to you to consider. Great Job!

src/dag_algo/mod.rs

raynelfss · 2023-07-06T18:49:07Z

tests/rustworkx_tests/digraph/test_transitive_reduction.py

You might want to add more tests, just to make sure all bases are covered.

IvanIsCoding

The algorithm LGTM, we just need to address two things:

replace HashMap and HashSet with DictMap and IndexSet. It can be confusing to Python users why two calls to the same function yield different results. DictMap helps keep our results more deterministic
we need more unit test, some worthwhile cases include graphs with deleted nodes

src/dag_algo/mod.rs

…index_map for cases where there were nodes removed. Added tests.

… have removed nodes

…gie/rustworkx into transitive-reduction

mtreinish

This is a good start, thanks for doing this. I think most of the implementation looks correct. I left some inline comments though on some improvements that could be made that I think will make it faster.

mtreinish · 2023-08-08T19:59:38Z

src/dag_algo/mod.rs

+///
+/// :param PyDiGraph graph: A directed acyclic graph
+///
+/// :returns: a directed acyclic graph representing the transitive reduction, and a map containing the index of a node in the original graph mapped to its equivalent in the resulting graph.


Suggested change

/// :returns: a directed acyclic graph representing the transitive reduction, and a map containing the index of a node in the original graph mapped to its equivalent in the resulting graph.

/// :returns: a directed acyclic graph representing the transitive reduction, and

/// a map containing the index of a node in the original graph mapped to its

/// equivalent in the resulting graph.

mtreinish · 2023-08-08T20:00:58Z

src/dag_algo/mod.rs

+/// :param PyDiGraph graph: A directed acyclic graph
+///
+/// :returns: a directed acyclic graph representing the transitive reduction, and a map containing the index of a node in the original graph mapped to its equivalent in the resulting graph.
+/// :rtype: Tuple[:class:`~rustworkx.PyGraph`, dict]


I think sphinx is smart enough to resolve this from rtype:

Suggested change

/// :rtype: Tuple[:class:`~rustworkx.PyGraph`, dict]

/// :rtype: Tuple[PyGraph, dict]

mtreinish · 2023-08-08T20:03:12Z

src/dag_algo/mod.rs

+
+/// Returns the transitive reduction of a directed acyclic graph
+///
+/// The transitive reduction of G = (V,E) is a graph G- = (V,E-)


Suggested change

/// The transitive reduction of G = (V,E) is a graph G- = (V,E-)

/// The transitive reduction of :math:`G = (V, E)` is a graph :math:`G\prime = (V, E\prime)`

mtreinish · 2023-08-08T20:05:20Z

src/dag_algo/mod.rs

+/// Returns the transitive reduction of a directed acyclic graph
+///
+/// The transitive reduction of G = (V,E) is a graph G- = (V,E-)
+/// such that for all v,w in V there is an edge (v,w) in E- if and only if (v,w) is in E


Suggested change

/// such that for all v,w in V there is an edge (v,w) in E- if and only if (v,w) is in E

/// such that for all :math:`v` and :math:`w` in :math:`V` there is an edge :math:`(v, w)` in

/// :math:`E\prime` if and only if :math:`(v, w)` is in :math:`E`

mtreinish · 2023-08-08T20:05:42Z

src/dag_algo/mod.rs

+///
+/// The transitive reduction of G = (V,E) is a graph G- = (V,E-)
+/// such that for all v,w in V there is an edge (v,w) in E- if and only if (v,w) is in E
+/// and there is no path from v to w in G with length greater than 1.


Suggested change

/// and there is no path from v to w in G with length greater than 1.

/// and there is no path from :math:`v` to :math:`w` in :math:`G` with length greater than 1.

mtreinish · 2023-08-08T20:09:23Z

src/dag_algo/mod.rs

+    py: Python,
+) -> PyResult<(digraph::PyDiGraph, DictMap<usize, usize>)> {
+    let g = &graph.graph;
+    let mut index_map = DictMap::new();


Suggested change

let mut index_map = DictMap::new();

let mut index_map = DictMap::with_capacity(g.node_count());

mtreinish · 2023-08-08T20:12:32Z

src/dag_algo/mod.rs

+            "Directed Acyclic Graph required for transitive_reduction",
+        ));
+    }
+    let mut tr = StablePyGraph::<Directed>::new();


Suggested change

let mut tr = StablePyGraph::<Directed>::new();

let mut tr = StablePyGraph::<Directed>::with_capacity(g.node_count(), 0);

tests/rustworkx_tests/digraph/test_transitive_reduction.py

mtreinish · 2023-08-08T20:17:41Z

tests/rustworkx_tests/digraph/test_transitive_reduction.py

+        graph2 = rustworkx.PyDiGraph()
+        a = graph2.add_node("a")
+        b = graph2.add_node("b")
+        c = graph2.add_node("c")
+        graph2.add_edges_from(
+            [
+                (a, b, 1),
+                (b, c, 1),
+                (a, c, 1),
+            ]
+        )
+        tr2, _ = rustworkx.transitive_reduction(graph2)
+        self.assertCountEqual(list(tr2.edge_list()), [(0, 1), (1, 2)])
+
+        graph3 = rustworkx.PyDiGraph()
+        graph3.add_nodes_from([0, 1, 2, 3])
+        graph3.add_edges_from([(0, 1, 1), (0, 2, 1), (0, 3, 1), (1, 2, 1), (1, 3, 1)])
+        tr3, _ = rustworkx.transitive_reduction(graph3)
+        self.assertCountEqual(list(tr3.edge_list()), [(0, 1), (1, 2), (1, 3)])


Should this be 3 tests?

mtreinish · 2023-08-08T20:18:18Z

tests/rustworkx_tests/digraph/test_transitive_reduction.py

+
+        graph.remove_node(3)
+
+        tr, _ = rustworkx.transitive_reduction(graph)


I think this should have an assertion on the index map being correct since c is removed.

- Fixing Docs - Fixing Maps to only have capacity of node_count - Fixing tests

mtreinish

This LGTM, thanks for the quick update. I think this is all ready to go from the code. THe only thing that I think is missing is we're not including this function in the docs. You just need to add this function to the index in: https://github.com/Qiskit/rustworkx/blob/main/docs/source/api/algorithm_functions/dag_algorithms.rst

mtreinish

This LGTM now, thanks for all the updates and sorry for the delay in review.

* Implementing filter_nodes and filter_edges funcs * Running fmt and clippy * Fixed issue where errors were not being propagated up to Python. Created tests for filter_edges and filter_nodes for both PyGraph and PyDiGraph. Created release notes for the functions. * Ran fmt, clippy, and tox * Fixing release notes * Fixing release notes again * Fixing release notes again again * Fixed release notes * Fixed release notes. Changed Vec allocation. Expanded on documentation. * ran cargo fmt and clippy * working on adding different parallel edge behavior * Fixing docs for filter functions * Working on graph_adjacency_matrix * Implementing changes to graph_adjacency_matrix and digraph_adjacency_matrix * working on release notes * Fixed release notes and docs * Ran cargo fmt * Ran cargo clippy * Fixed digraph_adjacency_matrix, passes tests * Removed mpl_draw from r elease notes * Changed if-else blocks in adjacency_matrix functions to match blocks. Wrote tests. * Fixed tests to pass lint * Added transitive reduction function to dag algo module * Fixed issue with graph that have nodes removed. Function now returns index_map for cases where there were nodes removed. Added tests. * Changing graph.nodes_removed to be false again. Return graph does not have removed nodes * Adding requested changes: - Fixing Docs - Fixing Maps to only have capacity of node_count - Fixing tests * Adding function to DAG algorithsm index

danielleodigie and others added 30 commits May 31, 2023 12:40

Implementing filter_nodes and filter_edges funcs

b88741d

Merge branch 'Qiskit:main' into main

a7de5ad

Running fmt and clippy

ca01739

Merge branch 'main' of https://github.com/danielleodigie/rustworkx

06119be

Fixed issue where errors were not being propagated up to Python. Crea…

1a0f219

…ted tests for filter_edges and filter_nodes for both PyGraph and PyDiGraph. Created release notes for the functions.

Ran fmt, clippy, and tox

a5301ef

Fixing release notes

8ed4220

Merge branch 'Qiskit:main' into main

7f8b655

Fixing release notes again

363dcb4

Fixing release notes again again

9e13637

Fixed release notes

496e1e8

Fixed release notes. Changed Vec allocation. Expanded on documentation.

ece5717

ran cargo fmt and clippy

193e55a

working on adding different parallel edge behavior

fa60e89

Fixing docs for filter functions

284b4d0

Merge branch 'main' into main

f3ba571

Working on graph_adjacency_matrix

a9df47d

Implementing changes to graph_adjacency_matrix and digraph_adjacency_…

0be3566

…matrix

working on release notes

ef783a5

Merge branch 'Qiskit:main' into adjacency-matrix

07fdde9

Fixed release notes and docs

131759d

Ran cargo fmt

d8763f7

Ran cargo clippy

f2cfc58

Fixed digraph_adjacency_matrix, passes tests

ae0515e

Removed mpl_draw from r

30c4687

elease notes

Changed if-else blocks in adjacency_matrix functions to match blocks.…

80c1c99

… Wrote tests.

Merge branch 'main' into adjacency-matrix

d8e35eb

Fixed tests to pass lint

de61f7a

Merge branch 'adjacency-matrix' of https://github.com/danielleodigie/…

c06559a

…rustworkx into adjacency-matrix

Merge branch 'Qiskit:main' into main

b28f1ac

danielleodigie and others added 3 commits July 6, 2023 10:10

Added transitive reduction function to dag algo module

506baaf

Merge branch 'Qiskit:main' into transitive-reduction

f89f547

Merge branch 'transitive-reduction' of https://github.com/danielleodi…

9a78924

…gie/rustworkx into transitive-reduction

raynelfss approved these changes Jul 6, 2023

View reviewed changes

IvanIsCoding reviewed Jul 8, 2023

View reviewed changes

src/dag_algo/mod.rs Outdated Show resolved Hide resolved

src/dag_algo/mod.rs Outdated Show resolved Hide resolved

src/dag_algo/mod.rs Outdated Show resolved Hide resolved

src/dag_algo/mod.rs Outdated Show resolved Hide resolved

danielleodigie and others added 5 commits July 13, 2023 09:55

Fixed issue with graph that have nodes removed. Function now returns …

b961f4f

…index_map for cases where there were nodes removed. Added tests.

Merge branch 'Qiskit:main' into transitive-reduction

2ac4498

Changing graph.nodes_removed to be false again. Return graph does not…

7a4ea26

… have removed nodes

Merge branch 'transitive-reduction' of https://github.com/danielleodi…

250876f

…gie/rustworkx into transitive-reduction

Merge branch 'Qiskit:main' into transitive-reduction

6aa24dc

mtreinish reviewed Aug 8, 2023

View reviewed changes

danielleodigie and others added 2 commits August 8, 2023 17:49

Adding requested changes:

64dc66b

- Fixing Docs - Fixing Maps to only have capacity of node_count - Fixing tests

Merge branch 'main' into transitive-reduction

2dc72e5

mtreinish reviewed Aug 9, 2023

View reviewed changes

Adding function to DAG algorithsm index

32b9abf

mtreinish approved these changes Aug 9, 2023

View reviewed changes

mtreinish merged commit f1de4bc into Qiskit:main Aug 9, 2023
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Transitive Reduction Function #923

Adding Transitive Reduction Function #923

danielleodigie commented Jul 6, 2023

coveralls commented Jul 6, 2023 •

edited

raynelfss left a comment

raynelfss Jul 6, 2023

IvanIsCoding left a comment

mtreinish left a comment

mtreinish Aug 8, 2023

mtreinish Aug 8, 2023

mtreinish Aug 8, 2023

mtreinish Aug 8, 2023

mtreinish Aug 8, 2023

mtreinish Aug 8, 2023

mtreinish Aug 8, 2023

mtreinish Aug 8, 2023

mtreinish Aug 8, 2023

mtreinish left a comment

mtreinish left a comment

	/// :rtype: Tuple[:class:`~rustworkx.PyGraph`, dict]
	/// :rtype: Tuple[PyGraph, dict]

	/// The transitive reduction of G = (V,E) is a graph G- = (V,E-)
	/// The transitive reduction of :math:`G = (V, E)` is a graph :math:`G\prime = (V, E\prime)`

	/// such that for all v,w in V there is an edge (v,w) in E- if and only if (v,w) is in E
	/// such that for all :math:`v` and :math:`w` in :math:`V` there is an edge :math:`(v, w)` in
	/// :math:`E\prime` if and only if :math:`(v, w)` is in :math:`E`

	/// and there is no path from v to w in G with length greater than 1.
	/// and there is no path from :math:`v` to :math:`w` in :math:`G` with length greater than 1.

	let mut index_map = DictMap::new();
	let mut index_map = DictMap::with_capacity(g.node_count());

	let mut tr = StablePyGraph::<Directed>::new();
	let mut tr = StablePyGraph::<Directed>::with_capacity(g.node_count(), 0);


		graph.remove_node(3)

		tr, _ = rustworkx.transitive_reduction(graph)

Adding Transitive Reduction Function #923

Adding Transitive Reduction Function #923

Conversation

danielleodigie commented Jul 6, 2023

coveralls commented Jul 6, 2023 • edited

Pull Request Test Coverage Report for Build 5811368748

💛 - Coveralls

raynelfss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanIsCoding left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

coveralls commented Jul 6, 2023 •

edited