-
Notifications
You must be signed in to change notification settings - Fork 132
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 "saturation first" and "independent set" greedy node coloring strategies #1138
Conversation
Pull Request Test Coverage Report for Build 9294148578Details
💛 - Coveralls |
I am confused about the "stubs" problems: what are these and how should I fix them?
|
I think I have addressed all of the comments from the previous review, this is ready for another round! :) |
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.
Overall this is looking good. I left a few inline code suggestions but the basic algorithms look sound. The only things I think we need to settle on is the naming of some of the new interfaces. The other concern I have is around backwards compatibility for the public api for the function in rustworkx-core. The modifications to the interfaces for the existing functions are all potentially backwards incompatible and could cause issues for users upgrading. It might be better to split off the strategy mode to yet another new function to avoid that. (Although having 3 functions isn't ideal either)
Also I think this is missing a release note. Nevermind it's the first file I looked at
releasenotes/notes/saturation-greedy-color-109d40f189590d3a.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Thanks @mtreinish, as suggested (and later discussed offline) I have retained the older version of the coloring functions in rustworkx-core and removed using the unnecessary |
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.
Overall this looks great, thanks for all the diligent work to update things and fix the backwards compatibility. I just have one inline question about the API for the new functions in rustworkx-core (it applies to both the nodes and edges functions). Otherwise I think this is good to merge.
/// | ||
pub fn greedy_edge_color_with_coloring_strategy<G, F, E>( | ||
graph: G, | ||
preset_color_fn: F, |
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.
Is there a reason to not make this Option<F>
since it's not mandatory. It just looks a bit awkward in the rustworkx
crate side to handle the case when there isn't preset.
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.
Thanks! I definitely agree it's better to make it Option<F>
. With this change, the function above would look like
pub fn greedy_edge_color_with_coloring_strategy<G, F, E>(
graph: G,
preset_color_fn: Option<F>,
strategy: ColoringStrategy,
) -> Result<DictMap<G::EdgeId, usize>, E>
where
G: EdgeCount + IntoNodeIdentifiers + IntoEdges,
G::EdgeId: Hash + Eq,
F: Fn(G::EdgeId) -> Result<Option<usize>, E>,
So far so good. I thought that using this function (from say testing) now becomes something like
let preset_color_fn = |node_idx: EdgeIndex| -> Result<Option<usize>, Infallible> {
if node_idx.index() == 1 {
Ok(Some(1))
} else {
Ok(None)
}
};
let colors = greedy_edge_color_with_coloring_strategy(
&graph,
Some(preset_color_fn),
ColoringStrategy::Degree,
)
.unwrap();
for the case that we want to specify the preset coloring function, and
let colors = greedy_edge_color_with_coloring_strategy(
&graph,
None,
ColoringStrategy::Degree,
)
.unwrap();
for the case that we do not. However the compiler does not like the second case:
--> rustworkx-core\src\coloring.rs:1505:13
|
1502 | let colors = greedy_edge_color_with_coloring_strategy(
| ---------------------------------------- required by a bound introduced by this call
...
1505 | None,
| ^^^^ cannot infer type of the type parameter `T` declared on the enum `Option`
|
= note: multiple `impl`s satisfying `_: Fn<(petgraph::prelude::EdgeIndex,)>` found in the following crates: `alloc`, `core`:
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `greedy_edge_color_with_coloring_strategy`
help: consider specifying the generic argument
|
1505 | None::<T>,
| +++++
and I can't figure out how to make this work.
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.
Ok, yeah I can see why rust isn't happy. It needs to know what the Some()
branch is in that case so it can generate the code for the generic usage in this case. But None
doesn't have the context and specifying the type with None::<T>
would be super ugly, especially for a fn generic. I thought it would be more ergonomic to be able to do None
here, but I guess if it's not that easy. Let's just leave this as is.
Description
This PR adds two more standard coloring strategies to greedy node/edge coloring functions
graph_greedy_color
andgraph_greedy_edge_color
.In the literature the first strategy is known as "saturation", "DSATUR" or "SLF". We dynamically choose the vertex that has the largest number of different colors already assigned to its neighbors, and, in case of a tie, the vertex that has the largest number of uncolored neighbors.
The second strategy is known as "greedy independent sets" or "GIS". We greedily find independent subsets of the graph, and assign a different color to each of these subsets.
This addresses #1137, modulo the fact that there are quadrillions of different greedy node coloring algorithms, and each comes with trillion variations.
Both new strategies can be combined with
preset_color_fn
(for node coloring).There is also a new Rust enum
GreedyStrategy
exposed as a python class that allows to specify which of the three currently supported greedy strategies is used. This is, calling the functions from the Python code would be as follows:Update: the
greedy_graph_edge_color
function has been also extended to support thepreset_color_fn
argument (though in this case it's an optional map from an edge index to either a color or toNone
)