-
Notifications
You must be signed in to change notification settings - Fork 145
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
Adding Transitive Reduction Function #923
Conversation
…ted tests for filter_edges and filter_nodes for both PyGraph and PyDiGraph. Created release notes for the functions.
elease notes
…rustworkx into adjacency-matrix
Pull Request Test Coverage Report for Build 5811368748
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything here looks great to me. I left a couple comments that are up to you to consider. Great Job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to add more tests, just to make sure all bases are covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithm LGTM, we just need to address two things:
…index_map for cases where there were nodes removed. Added tests.
… have removed nodes
…gie/rustworkx into transitive-reduction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/dag_algo/mod.rs
Outdated
/// | ||
/// :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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// :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. |
src/dag_algo/mod.rs
Outdated
/// :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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think sphinx is smart enough to resolve this from rtype
:
/// :rtype: Tuple[:class:`~rustworkx.PyGraph`, dict] | |
/// :rtype: Tuple[PyGraph, dict] |
src/dag_algo/mod.rs
Outdated
|
||
/// Returns the transitive reduction of a directed acyclic graph | ||
/// | ||
/// The transitive reduction of G = (V,E) is a graph G- = (V,E-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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)` |
src/dag_algo/mod.rs
Outdated
/// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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` |
src/dag_algo/mod.rs
Outdated
/// | ||
/// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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. |
src/dag_algo/mod.rs
Outdated
py: Python, | ||
) -> PyResult<(digraph::PyDiGraph, DictMap<usize, usize>)> { | ||
let g = &graph.graph; | ||
let mut index_map = DictMap::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut index_map = DictMap::new(); | |
let mut index_map = DictMap::with_capacity(g.node_count()); |
src/dag_algo/mod.rs
Outdated
"Directed Acyclic Graph required for transitive_reduction", | ||
)); | ||
} | ||
let mut tr = StablePyGraph::<Directed>::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut tr = StablePyGraph::<Directed>::new(); | |
let mut tr = StablePyGraph::<Directed>::with_capacity(g.node_count(), 0); |
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)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be 3 tests?
|
||
graph.remove_node(3) | ||
|
||
tr, _ = rustworkx.transitive_reduction(graph) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Aims to fix #801
Adds transitive_reduction function to DAG algorithms module