Additions and modifications to greedy_coloring and equitable_coloring #7423
Conversation
- Use of more contemporary algorithms for graph colouring - Improved documentation - Improved performance and simplified code for several existing methods
There was a problem hiding this comment.
For starters, it'd be good to ensure that any of the tests that are currently in place haven't been removed or modified. This is important in order to ensure that the changes are backward-compatible.
It's hard to tell from the diff whether there are material changes or whether these are just style issues, so the first step is resolving the style problems. A quick recipe:
pip install pre-commit
pre-commit install
pre-commit run --all-filesSee the contributor guide for further details!
|
Thank you for your advice,
The code changes I've proposed are adding some flexibility to the library,
allowing several options to be taken that were not previously allowed (e.g.
using the "DSATUR" option to be used while "interchange=True").
This extra flexibility makes a couple of existing tests unnecessary. If I
am not allowed to remove these tests, what would be your advice here?
Sorry, but I've not done this before.
Thank you
Rhyd Lewis
…On Thu, 18 Apr 2024 at 17:05, Ross Barnowski ***@***.***> wrote:
***@***.**** commented on this pull request.
For starters, it'd be good to ensure that any of the tests that are
currently in place haven't been removed or modified. This is important in
order to ensure that the changes are backward-compatible.
It's hard to tell from the diff whether there are material changes or
whether these are just style issues, so the first step is resolving the
style problems. A quick recipe:
pip install pre-commit
pre-commit install
pre-commit run --all-files
See the contributor guide
<https://networkx.org/documentation/latest/developer/contribute.html#development-workflow>
for further details!
—
Reply to this email directly, view it on GitHub
<#7423 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BG545UU253RSP6RYATM32NDY57VNRAVCNFSM6AAAAABGNJSHY2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMBZGMZDIOBZGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
No worries, @Rhyd-Lewis - and thanks for the proposal! As mentioned above, the first step will be to fix the style issues: at this point, it's not clear to reviewers how extensive the changes to the test suite actually are, as the modifications are intertwined with style/reformatting changes. Once those are rectified (see procedure in the previous comment, or the contributor guide it will be more straightforward to tell whether there are any backwards-incompatible changes and to what degree (ha!). At a higher level - the reason why this constraint exists is that these functions are already in use by users for all sorts of applications. It's important to ensure that any changes that go into the library don't silently yank the rug out from under them and change the behavior of existing code, at least not without some form of forewarning. |
…te that test_coloring.py had been altered because (a) the new version allows some paramter combinations that were previously disallowed, and (b) additional parameter options are also available for greedy_coloring and equitable_coloring. I hope that this is OK At a higher level - the reason why this constraint exists is that these functions are already in use by users for all sorts of applications. It's important to ensure that any changes that go into the library don't silently yank the rug out from under them and change the behavior of existing code, at least not without some form of forewarning.
|
Thanks for the information Ross,
I've sorted the style issues. I've also ensured (in the tests and
otherwise) that the new functions are backwards compatible with the
existing ones.
Happy to address any feedback that you have
Rhyd
…On Tue, 23 Apr 2024 at 18:24, Ross Barnowski ***@***.***> wrote:
No worries, @Rhyd-Lewis <https://github.com/Rhyd-Lewis> - and thanks for
the proposal!
As mentioned above, the first step will be to fix the style issues: at
this point, it's not clear to reviewers how extensive the changes to the
test suite actually are, as the modifications are intertwined with
style/reformatting changes. Once those are rectified (see procedure in the
previous comment, or the contributor guide
<https://networkx.org/documentation/latest/developer/contribute.html> it
will be more straightforward to tell whether there are any
backwards-incompatible changes and to what degree (ha!).
At a higher level - the reason why this constraint exists is that these
functions are already in use by users for all sorts of applications. It's
important to ensure that any changes that go into the library don't
silently yank the rug out from under them and change the behavior of
*existing* code, at least not without some form of forewarning.
—
Reply to this email directly, view it on GitHub
<#7423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BG545UQCPF6DT3VRKUQY4FTY62KMHAVCNFSM6AAAAABGNJSHY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZSHE4DCOJRGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hello. All tests (except for reviewer label) have now been passed. Looking forward to receiving any feedback. Thanks |
|
I took a look at the PR and I think it is changing the interface in a way I find confusing at first glance. Perhaps I am missing something -- or maybe we just need to make a new function instead of changing an existing function. Making a new function could also make this easier to review -- (it looks like almost every line has changed, so it's hard to tell what this does to t he existing function). My questions:
minor nit: can you use |
|
Hi Dan,
Thanks for your message and your questions. I appreciate you giving your
time
Answer 1) Currently the equitable coloring algorithms on NetworkX only
operate if the user specifies a number of colours that is larger than the
maximum degree of the graph. (It produces an exception otherwise). The
algorithm then produces a perfectly equitable colouring. In practice, using
a smaller number of colours leads to an NP-hard problem; however, equitable
colourings are still achievable in many cases. My code has simply added to
this -- the new heuristic is only used in the cases where the existing
algorithm would have returned an exception. I've provided a few more
details (on complexity and accuracy) in the first post above.
Answer 2) Yes, it would be possible to add my new (tabu search) strategy
without removing the existing "interchange" strategy. However, the
interchange strategy comes from a book written in the early 1980s and is
quite slow and inaccurate compared to contemporary approaches (evidence for
this is provided in the first post above). Finally, for some reason, the
interchange operator only works with some of the greedy methods listed in
this file and returns exceptions otherwise. My thoughts were that it would
be tidier to just get rid of this approach because the new tabu search
algorithm rectifies all of these problems. For backwards compatibility I
have, however, kept the "interchange" option in the API. If it is selected,
it simply calls the tabu search algorithm for n iterations. The first post
above shows how this leads to better results and run times.
So, these were my thoughts. But as this is my first time contributing to
NetworkX, I am happy to follow your recommendations. Let me know your
advice, and I will endeavour to follow it.
Rhyd Lewis
PS: FYI, the book that I have referenced in the code is my own:
Lewis, R. (2021) A Guide to Graph Colouring: Algorithms and Applications,
2nd Ed. Springer, ISBN: 978-3-030-81053-5
https://link.springer.com/book/10.1007/978-3-030-81054-2.
…On Wed, 8 May 2024 at 16:04, Dan Schult ***@***.***> wrote:
I took a look at the PR and I think it is changing the interface in a way
I find confusing at first glance. Perhaps I am missing something -- or
maybe we just need to make a new function instead of changing an existing
function. Making a new function could also make this easier to review --
(it looks like almost every line has changed, so it's hard to tell what
this does to t he existing function).
My questions:
- this seems to provide a heuristic method that (sometimes) does not
return an equitable coloring. That seems confusing given the name of the
function.
- this seems to add a few new strategies for coloring -- but the diff
makes it look like you are also changing/replacing an existing strategy.
Can you make the additions without removing any existing strategies? Even
if the strategy is essentially the same as an existing one, it is probably
better to add a new one and leave the old so people counting on it can
continue to use it.
minor nit: can you use log(m) instead of lg m for the documentation of
complexity? push back if that's not ok.
—
Reply to this email directly, view it on GitHub
<#7423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BG545UTFTEZ4W5XA3TEK4FLZBI5ITAVCNFSM6AAAAABGNJSHY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBQG44TQMZWGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hi All, Awaiting feedback on most recent commit. Thanks Rhyd |
There was a problem hiding this comment.
I think probably the easiest path forward here to to try to decompose the changes into multiple PRs - see comments below. The changes generally look good, but they are extensive and should undergo careful review! Most of my suggestions at this stage are about making that review easier!
| else: | ||
| r_ = 0 | ||
| # Employ the exact algorithm of [1]. First, map nodes to integers for |
There was a problem hiding this comment.
AFAICT the else is no longer necessary - getting rid of it and resetting the indent level from L531 onward would be a big help I think!
|
|
||
| The strategies are described in [1]_, and smallest-last is based on | ||
| [2]_. | ||
| def greedy_color(G, strategy="largest_first", tabu=0, interchange=False): |
There was a problem hiding this comment.
The new kwarg should be added at the end of the signature in order to prevent breaking changes in existing code which rely on positional arguments. See the contributor guide for further details if interested!
|
|
||
| class TestColoring: | ||
| def test_basic_cases(self): |
There was a problem hiding this comment.
This test suite is certainly hard to follow and the efforts to improve it are much appreciated! However, with the removal/reorganization of tests it becomes very difficult to evaluate whether the changes to the code are not breaking tested cases.
The best way to push this forward IMO would be to split out test suite modifications (i.e. changes and removals) to a separate PR that is dedicated to refactor the test suite. New tests can (and should) be added to this PR, but removing/changing existing tests should be left off until after the proposed changes can be shown to pass the existing test suite as is (plus any new tests).
There was a problem hiding this comment.
Here are some comments on the equitable_coloring.py portion of the PR.
For the greedy coloring (as for the others too) we need to keep the old behavior -- including default behavior for any existing functions. At least through a deprecation cycle. I haven't worked through the implications of that because it looks like a lot has been removed that perhaps doesn't need to be. It would be helpful if you passed through the PR with a view toward backward compatibility and made it more clear what is changed and how users get the old behavior. Introducing new functions can be part of that -- you don't have to rewrite existing functions.
Thanks very much...
[Edit: Sorry -- I didn't see @rossbar's comments before I posted this.]
| c = equitable_heuristic(G, num_colors) | ||
| return c |
There was a problem hiding this comment.
Please replace this with the single line:
return equitable_heuristic(G, num_colors)
Then, since this is a "return" you can remove the else: line and dedent the rest of the code back to its original indentation level. That should help with the review too since only the changes (instead of indented) lines will show in github's diff.
| (2010). A fast algorithm for equitable coloring. Combinatorica, 30(2), | ||
| 217-224. | ||
| .. [2] Lewis, R. (2021) A Guide to Graph Colouring: Algorithms and | ||
| Applications, 2nd Ed. Springer, ISBN: 978-3-030-81053-5 | ||
| <https://link.springer.com/book/10.1007/978-3-030-81054-2> |
There was a problem hiding this comment.
very minor nit: The indentation should be 4 characters even if that doesn't match the first line's .. [1] indentation.
| if nx.is_directed(G): | ||
| raise nx.NetworkXNotImplemented("input graph cannot be directed.") |
There was a problem hiding this comment.
This is code that is run by the decorator not_implemented_for just above the function signature.
The use of that decorator makes this not necessary.
| """ | ||
| if nx.is_directed(G): | ||
| raise nx.NetworkXNotImplemented("input graph cannot be directed.") | ||
| if not isinstance(num_colors, int) or num_colors < 0: |
There was a problem hiding this comment.
Checking the type of the input as int means integer values that are not of Python int type wont work as input. One way to handle this is to use if int(num_colors) != num_colors .... But there are many other methods -- including not checking for it and seeing if the resulting exception is sufficiently clear to not need an explicit error here.
There was a problem hiding this comment.
Here is an example in networkx that checks whether an input is integral:
networkx/networkx/generators/classic.py
Line 805 in fd27fb0
This works with numpy integers:
>>> isinstance(np.int64(5), int)
False
>>> isinstance(np.int64(5), numbers.Integral)
TrueI think this works b/c numpy registers the int type here.
NumPy uses a different method to check for integers, such as:
import operator
try:
operator.index(maybe_integer)
except TypeError:
raise TypeError('blah blah') from NoneNumPy uses this trick lots of places, such as here:
https://github.com/numpy/numpy/blob/9142f32573612d544b65649cf6f485f55b48e97a/numpy/lib/_npyio_impl.py#L822-L828
This works b/c integral objects are those that implement __index__.
There was a problem hiding this comment.
I'd just as soon remove the isinstance check entirely - the docstring is very clear that num_colors has to be a positive integer, and a very clear exception would be raised immediately without this check:
>>> G = nx.dodecahedral_graph()
>>> nx.equitable_coloring(G, "n")
Traceback (most recent call last)
...
File ~/repos/networkx/networkx/algorithms/coloring/equitable_coloring.py:510, in equitable_color(G, num_colors)
507 # Calculate maximum degree in G
508 r_ = max(G.degree(node) for node in G.nodes) if len(G.nodes) > 0 else 0
--> 510 if num_colors <= r_:
511 # Employ the heuristic from [2]
512 c = equitable_heuristic(G, num_colors)
513 return c
TypeError: '<=' not supported between instances of 'str' and 'int'
|
Thanks for the comments. I'm on leave for the next week, but will step to
it as soon as I'm back. Thanks! Rhyd
…On Fri, 24 May 2024 at 18:17, Ross Barnowski ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In networkx/algorithms/coloring/equitable_coloring.py
<#7423 (comment)>:
>
- for idx, node in enumerate(G.nodes):
- nodes_to_int[node] = idx
- int_to_nodes[idx] = node
-
- G = nx.relabel_nodes(G, nodes_to_int, copy=True)
-
- # Basic graph statistics and sanity check.
- if len(G.nodes) > 0:
- r_ = max(G.degree(node) for node in G.nodes)
+ """
+ if nx.is_directed(G):
+ raise nx.NetworkXNotImplemented("input graph cannot be directed.")
+ if not isinstance(num_colors, int) or num_colors < 0:
I'd just as soon remove the isinstance check entirely - the docstring is
very clear that num_colors has to be a positive integer, and a very clear
exception would be raised immediately without this check:
>>> G = nx.dodecahedral_graph()>>> nx.equitable_coloring(G, "n")Traceback (most recent call last)
...File ~/repos/networkx/networkx/algorithms/coloring/equitable_coloring.py:510, in equitable_color(G, num_colors)
507 # Calculate maximum degree in G
508 r_ = max(G.degree(node) for node in G.nodes) if len(G.nodes) > 0 else 0--> 510 if num_colors <= r_:
511 # Employ the heuristic from [2]
512 c = equitable_heuristic(G, num_colors)
513 return c
TypeError: '<=' not supported between instances of 'str' and 'int'
—
Reply to this email directly, view it on GitHub
<#7423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BG545URS4B7M54TK3NKTK33ZD5Y3FAVCNFSM6AAAAABGNJSHY2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZXGQ3TGMRYGA>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Summary of changes. The following files have been changed. These are considered below, with further details
…\networkx\networkx\algorithms\coloring\equitable_colouring.py
As before, this new code provides an equitable k-colouring for the nodes of a graph G (where k is specified by the user via the parameter “num_colors”). The original version of this code only works when the number of colours k (defined by the user) exceeds the maximum degree of G. In our new version, a heuristic algorithm is added that is used in cases where k is less than this value. This allows more flexibility for users. This heuristic algorithm is described and documented in my book:
Lewis, R. (2021) A Guide to Graph Colouring: Algorithms and Applications, 2nd Ed. Springer, ISBN: 978-3-030-81053-5 https://link.springer.com/book/10.1007/978-3-030-81054-2.
The implementation of this new algorithm has complexity O((n log n) + (nk) + (m log m)). As required, in solutions returned by this method, neighbouring vertices always receive different colours; however, the colouring is not guaranteed to be equitable.
Several new exceptions have now also been added to this code to reflect these changes and to make the existing code safer.
The documentation and comments have been modified to reflect these changes. All other parts of the code remain the same, albeit with some tidying.
…\networkx\networkx\algorithms\coloring\greedy_colouring.py
Updates have also been made to the above file to improve performance and simplify the methods used for graph colouring. The main changes are:
Full descriptions and analyses of these algorithms can also be found in my book:
Lewis, R. (2021) A Guide to Graph Colouring: Algorithms and Applications, 2nd Ed. Springer, ISBN: 978-3-030-81053-5 https://link.springer.com/book/10.1007/978-3-030-81054-2.
A concise description of the new tabu search algorithm is given in the comments/documentation. It is a well-regarded contemporary method that is known to produce results close to the state of the art. Here are its main benefits.
The second change we have made is in the implementation if the Dsatur algorithm in NetworkX, where there is currently a slight error. In the original specification of Dsatur (given in the following paper):
Brélaz, D. (1979). "New methods to color the vertices of a graph". Communications of the ACM. 22(4), pp. 251–256.
the next vertex to be coloured should be the one with the highest saturation degree, breaking ties by selecting the vertex with the highest degree in the subgraph induced by the uncoloured vertices. However, the current NetworkX implementation breaks ties by only looking at the degree of the vertex in the original graph. This fault is corrected in our proposed implementation. Our new implementation also makes use of a priority queue and has a complexity of O((n+m) lg(n+m)) as opposed to the existing implementation of O(n^2). This new implementation occupies fewer lines of code and is faster for all but the densest of graphs. A further analysis of this algorithm (performance and complexity) is available in the above book.
Finally, we have also added the famous recursive largest first (RLF) algorithm as an option for the greedy colouring method. This is a well-known and high-performing method that has a complexity of O(nm). This is slightly more expensive than the existing greedy methods, although it is also known to produce better quality solutions. (Information on this can also be seen in the above book and at https://www.rhydlewis.eu/gcol/analysis.pdf.)
…\networkx\networkx\algorithms\coloring\tests\test_coloring.py
In the above file, several new tests have been added to ensure our new code functions as expected. We can confirm that is correct. Additional information on run times and performance can also be seen in https://www.rhydlewis.eu/gcol/analysis.pdf