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
6 changes: 6 additions & 0 deletions releasenotes/notes/migrate-greedy-color-c3239f35840eec18.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
features:
- |
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.

86 changes: 86 additions & 0 deletions rustworkx-core/src/coloring.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Licensed under the Apache License, Version 2.0 (the "License"); you may
// not use this file except in compliance with the License. You may obtain
// a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
// License for the specific language governing permissions and limitations
// under the License.

use std::cmp::Reverse;
use std::hash::Hash;

use crate::dictmap::*;
use hashbrown::{HashMap, HashSet};
use petgraph::visit::{EdgeRef, IntoEdges, IntoNodeIdentifiers, NodeCount};
use rayon::prelude::*;

/// Color a graph using a greedy graph coloring algorithm.
///
/// This function uses a `largest-first` strategy as described in [1]_ and colors
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
/// the nodes with higher degree first.
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
///
/// Arguments:
///
/// * `graph` - The graph object to run the algorithm on
///
/// # Example
/// ```rust
///
/// use petgraph::graph::Graph;
/// use petgraph::graph::NodeIndex;
/// use petgraph::Undirected;
/// use rustworkx_core::dictmap::*;
/// use rustworkx_core::coloring::greedy_color;
///
/// let g = Graph::<(), (), Undirected>::from_edges(&[(0, 1), (0, 2)]);
/// let colors = greedy_color(&g);
/// 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);
/// assert_eq!(colors, expected_colors);
/// ```
///
///
/// .. [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.

where
G: NodeCount + IntoNodeIdentifiers + IntoEdges,
G::NodeId: Hash + Eq + Send + Sync,
{
let mut colors: DictMap<G::NodeId, usize> = DictMap::new();
let mut node_vec: Vec<G::NodeId> = graph.node_identifiers().collect();

let mut sort_map: HashMap<G::NodeId, usize> = HashMap::with_capacity(graph.node_count());
for k in node_vec.iter() {
sort_map.insert(*k, graph.edges(*k).count());
}
node_vec.par_sort_by_key(|k| Reverse(sort_map.get(k)));

for node in node_vec {
let mut neighbor_colors: HashSet<usize> = HashSet::new();
for edge in graph.edges(node) {
let target = edge.target();
let existing_color = match colors.get(&target) {
Some(color) => color,
None => continue,
};
neighbor_colors.insert(*existing_color);
}
let mut current_color: usize = 0;
loop {
if !neighbor_colors.contains(&current_color) {
break;
}
current_color += 1;
}
colors.insert(node, current_color);
}

colors
}
2 changes: 2 additions & 0 deletions rustworkx-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ pub type Result<T, E = Infallible> = core::result::Result<T, E>;

/// Module for centrality algorithms.
pub mod centrality;
/// Module for coloring algorithms.
pub mod coloring;
pub mod connectivity;
pub mod generators;
/// Module for maximum weight matching algorithms.
Expand Down
51 changes: 51 additions & 0 deletions rustworkx-core/tests/coloring.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Licensed under the Apache License, Version 2.0 (the "License"); you may
// not use this file except in compliance with the License. You may obtain
// a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
// License for the specific language governing permissions and limitations
// under the License.

//! Test module for coloring algorithms.

use petgraph::graph::Graph;
use petgraph::graph::NodeIndex;
use petgraph::Undirected;
use rustworkx_core::dictmap::*;

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).

let graph = Graph::<(), (), Undirected>::new_undirected();
let colors = greedy_color(&graph);
let expected_colors = DictMap::new();
assert_eq!(colors, expected_colors);
}

#[test]
fn test_greedy_color_simple_graph() {
let graph = Graph::<(), (), Undirected>::from_edges(&[(0, 1), (0, 2)]);
let colors = greedy_color(&graph);
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).

assert_eq!(colors, expected_colors);
}

#[test]
fn test_greedy_color_simple_graph_large_degree() {
let graph =
Graph::<(), (), Undirected>::from_edges(&[(0, 1), (0, 2), (0, 2), (0, 2), (0, 2), (0, 2)]);
let colors = greedy_color(&graph);
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);
assert_eq!(colors, expected_colors);
}
42 changes: 4 additions & 38 deletions src/coloring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,12 @@
// under the License.

use crate::graph;
use rustworkx_core::dictmap::*;

use hashbrown::{HashMap, HashSet};
use std::cmp::Reverse;
use rustworkx_core::coloring::greedy_color;

use pyo3::prelude::*;
use pyo3::types::PyDict;
use pyo3::Python;

use petgraph::graph::NodeIndex;
use petgraph::prelude::*;
use petgraph::visit::NodeCount;

use rayon::prelude::*;

/// Color a :class:`~.PyGraph` object using a greedy graph coloring algorithm.
///
/// This function uses a `largest-first` strategy as described in [1]_ and colors
Expand Down Expand Up @@ -61,35 +52,10 @@ use rayon::prelude::*;
#[pyfunction]
#[pyo3(text_signature = "(graph, /)")]
pub fn graph_greedy_color(py: Python, graph: &graph::PyGraph) -> PyResult<PyObject> {
let mut colors: DictMap<usize, usize> = DictMap::new();
let mut node_vec: Vec<NodeIndex> = graph.graph.node_indices().collect();
let mut sort_map: HashMap<NodeIndex, usize> = HashMap::with_capacity(graph.node_count());
for k in node_vec.iter() {
sort_map.insert(*k, graph.graph.edges(*k).count());
}
node_vec.par_sort_by_key(|k| Reverse(sort_map.get(k)));
for u_index in node_vec {
let mut neighbor_colors: HashSet<usize> = HashSet::new();
for edge in graph.graph.edges(u_index) {
let target = edge.target().index();
let existing_color = match colors.get(&target) {
Some(node) => node,
None => continue,
};
neighbor_colors.insert(*existing_color);
}
let mut count: usize = 0;
loop {
if !neighbor_colors.contains(&count) {
break;
}
count += 1;
}
colors.insert(u_index.index(), count);
}
let colors = greedy_color(&graph.graph);
let out_dict = PyDict::new(py);
for (index, color) in colors {
out_dict.set_item(index, color)?;
for (node, color) in colors {
out_dict.set_item(node.index(), color)?;
}
Ok(out_dict.into())
}