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

moving greedy-color to rustworkx-core #875

Merged
merged 11 commits into from
May 30, 2023

Conversation

alexanderivrii
Copy link
Contributor

This PR moves the greedy_color algorithm to rustworkx_core. The algorithm is exactly the same modulo some minor renaming and replacement of node_indices by node_identifiers.

The rust tests in rustworkx-core/tests/coloring.rs are adapted from python tests in rustworkx_test/graph/test_coloring.py.


I am wondering if there is a way to replace

let mut expected_colors = DictMap::new();
expected_colors.insert(NodeIndex::new(0), 0);
expected_colors.insert(NodeIndex::new(1), 1);
expected_colors.insert(NodeIndex::new(2), 1);

by something like DictMap::from (I was not able to make this work).

@coveralls
Copy link

coveralls commented May 16, 2023

Pull Request Test Coverage Report for Build 5010849131

  • 32 of 32 (100.0%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.4%) to 96.557%

Files with Coverage Reduction New Missed Lines %
src/random_graph.rs 1 99.44%
rustworkx-core/src/generators/mod.rs 2 0%
Totals Coverage Status
Change from base Build 4999136137: -0.4%
Covered Lines: 14581
Relevant Lines: 15101

💛 - Coveralls

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.

This LGTM, thanks for doing this. Just a couple small inline comments and questions, but then I think this is ready to merge.

Comment on lines 34 to 37
let mut expected_colors = DictMap::new();
expected_colors.insert(NodeIndex::new(0), 0);
expected_colors.insert(NodeIndex::new(1), 1);
expected_colors.insert(NodeIndex::new(2), 1);
Copy link
Member

Choose a reason for hiding this comment

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

Since you asked somewhere you can do this construction with something like:

Suggested change
let mut expected_colors = DictMap::new();
expected_colors.insert(NodeIndex::new(0), 0);
expected_colors.insert(NodeIndex::new(1), 1);
expected_colors.insert(NodeIndex::new(2), 1);
let mut expected_colors: DictMap<NodeIndex, usize> = [
(NodeIndex::new(0), 0),
(NodeIndex::new(1), 1),
(NodeIndex::new(2), 1),
]
.into_iter()
.collect();

but manually calling insert works fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I like the suggested approach (and in this case we also don't need the mut keyword). I have updated the tests accordingly (and kept the original way in the docs example).

use rustworkx_core::coloring::greedy_color;

#[test]
fn test_greedy_color_empty_graph() {
Copy link
Member

Choose a reason for hiding this comment

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

I'd be tempted to put these 3 test files under

#[cfg(test)]
mod tests {

}

in rustworkx-core/src/coloring.rs since they're isolated to just testing that module. But it doesn't matter too much I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in our previous discussions, we want to put tests that are for only one module inside the module as you suggest. Putting new modules in tests is generally reserved for tests that are testing interactions of multiple modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I have moved the tests to the coloring module (and I can see these running, now that I have learned to run cargo test).

Comment on lines 4 to 6
The function ``greedy_color``has been migrated to the ``rustworkx-core``
crate in the ``coloring`` module. It colors a graph using a greedy
graph coloring algorithm.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The function ``greedy_color``has been migrated to the ``rustworkx-core``
crate in the ``coloring`` module. It colors a graph using a greedy
graph coloring algorithm.
Added a new function, ``greedy_color``, to ``rustworkx-core`` in a new
``coloring`` module. It colors a graph using a greedy
graph coloring algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

///
/// .. [1] Adrian Kosowski, and Krzysztof Manuszewski, Classical Coloring of Graphs,
/// Graph Colorings, 2-19, 2004. ISBN 0-8218-3458-4.
pub fn greedy_color<G>(graph: G) -> DictMap<G::NodeId, usize>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we want to call this greedy_node_color or greedy_node_coloring just to make it a bit clear what it's doing. I don't feel super strongly this name works fine, it's just something to think about while we're adding the rust version of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the name of the internal rust function to greedy_node_color. I don't feel strongly one way or another, but it might be slightly more clear if we are planning to add greedy_edge_color later.

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.

This LGTM now. Just a couple small inline comments about the documentation for the new function.

rustworkx-core/src/coloring.rs Outdated Show resolved Hide resolved
rustworkx-core/src/coloring.rs Outdated Show resolved Hide resolved
@mtreinish mtreinish added the automerge Queue a approved PR for merging label May 30, 2023
@mergify mergify bot merged commit 1518b1e into Qiskit:main May 30, 2023
@alexanderivrii alexanderivrii deleted the greedy-color-to-core branch February 9, 2024 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants