Add Misra-Gries edge coloring method

[alexanderivrii]

This is an attempt to add the Misra-Gries edge coloring algorithm to Rustworkx, following @ihincks's python code in #854 and wiki page https://en.wikipedia.org/wiki/Misra_%26_Gries_edge_coloring_algorithm. This is still raw, but I would be happy to get some initial feedback on which parts of the code could be implemented in more rust-friendly way, whether the names and inputs/outputs of different functions look ok, etc.

[mtreinish]

I left a few quick initial comments from a first scan through the PR. The other question I have is whether you want to add this to rustworkx-core or not? I think it's probably generally useful enough (and you did just add the node coloring function to the core crate) that it might make sense to put this in rustworkx-core from the start.

[mtreinish]

Just for fun you could do this as an iterator too, but not sure if is any faster. Something like:

Suggested change

	let used_colors = self.get_used_colors(u);
	let mut c: usize = 0;
	while used_colors.contains(&c) {
	c += 1;
	}
	c
	let used_colors = self.get_used_colors(u);
	(0..)
	.position(|color| used_colors.contains(&color))
	.unwrap()

[mtreinish]

Instead of doing this it'd probably be more efficient to use neighbors.iter().enumerate() for the for loop. Then we'd have the index for every step and position_z would have no lookup overhead. Right now this is O(n) for this lookup as you have to re-traverse neighbors on each iteration.

[alexanderivrii]

@mtreinish, thanks for the detailed feedback. I guess putting the functionality in rustworkx-core makes sense. And I really like the iterator-based way of implementing things.

[alexanderivrii]

I have made another round of changes, moving the new edge coloring function to rustworkx-core and changing the colors to be a vector indexed by usize (the internal value of EdgeIndex, which we can get using EdgeIndexable). Using a vector rather than a hash map leads to about 20% improvement in performance (which is not surprising). I don't understand the cause for CI failures, @mtreinish could you please take a look?

[coveralls]

Pull Request Test Coverage Report for Build 7479009283

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.007%) to 95.915%

---

Totals
Change from base Build 7451338673:	0.007%
Covered Lines:	15990
Relevant Lines:	16671

---

💛 - Coveralls

[mtreinish]

I think the ci failure was a temporary issue related to the release of a new version of something. I just updated the pr branch and it seemed to have work fine.

[mtreinish]

Overall this LGTM, I just had a few small questions and suggestions inline.

I am a bit worried about the complexity of some of the inner loops in the helper methods as there is a lot of nested looping and those all get called in an outer loop from run_algorithm(). I'll need to re-read the paper to see if that's just inherit to the algorithm. But either way for an initial implementation this is fine anyway, if it becomes a performance bottleneck we can always make improvements down the road.

[mtreinish]

Just to avoid unbound iteration as a potential source of bugs:

Suggested change

	let free_color: usize = (0..)
	let free_color: usize = (0..self.colors.len())

[alexanderivrii]

Fixed in 4e22ad7 using the stricter upper bound of max_node_degree + 1.

[mtreinish]

Could you just do self.colors.contains(&Some(c)) (or something like this, I might have got the position of & incorrect)? You'd have the same linear overhead.

More generally though I'm wondering if it makes more sense to store a HashSet of used colors in the MisraGries struct so you don't have to generate it on the fly every time. Then you could make this a O(1) lookup every time. It would require doing book keeping on adding or changing colors but it should be manageable.

[alexanderivrii]

I have followed the above suggestion in 4e22ad7, and indeed it gives a significant speedup on medium/large-sized complete graphs. E.g., on my laptop previously it took 16.95 seconds to color a complete graph over 90 vertices, and now it takes 0.58 seconds. For heavy-hex graphs the difference is less pronounced, but the new implementation is still a bit faster.

[mtreinish]

Suggested change

	let mut fan: Vec<(G::EdgeId, G::NodeId)> = Vec::new();
	fan.push((eid, v));
	let mut fan: Vec<(G::EdgeId, G::NodeId)> = vec![(eid, v)];

[alexanderivrii]

Done in 7900916. Also the code has been updated to use G::EdgeRef instead of G::EdgeId, so it's enough to only keep the edge reference.

[mtreinish]

The compiler should be able to infer the type here because it's defined on u.

Suggested change

	let mut cur_node: G::NodeId = u;
	let mut cur_node = u;

[alexanderivrii]

Removed in 7e6470d.

[mtreinish]

Oh, the other thing is the new function needs to be added to the api docs index here: https://github.com/Qiskit/rustworkx/blob/main/docs/source/api/algorithm_functions/other.rst (although maybe we should add a graph coloring page at this point)

[alexanderivrii]

I am a bit worried about the complexity of some of the inner loops in the helper methods as there is a lot of nested looping and those all get called in an outer loop from run_algorithm().

Agreed, it would be great if we could remove the extra loop in the get_maximal_fan function, for example by precomputing something at the beginning of the function. Essentially, given a node u and its neighbor node v we try to "extend" the fan by finding an edge (u, w) such that its color is not used at v. If such an edge is found, we add it to the fan, and repeat the above process with w instead of v. The problem is that the node v keeps changing and there are multiple colors that can be unused at a given node. So I don't see a simple way to remove one of the two loops. But would be happy to hear suggestions.

[mtreinish]

Yeah, I don't have any concrete suggestions off the top of my head yet either. It was more just my impressions as I was reviewing it. But like I said we can leave it like this and if performance becomes an issue or one of us comes up with something it's easy enough to change and improve the internal logic later.

[mtreinish]

LGTM, thanks for doing this.