feat : Add VNGraphs.jl stubs for chromatic_number and edge_chromatic_number#507
feat : Add VNGraphs.jl stubs for chromatic_number and edge_chromatic_number#507mahmudsudo wants to merge 4 commits intoJuliaGraphs:masterfrom
Conversation
Declares chromatic_number and edge_chromatic_number in Graphs.jl with helpful error hints directing users to install VNGraphs.jl for the fast C-backed very_nauty implementations. Includes unit tests asserting the stubs throw the expected errors.
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #507 +/- ##
=======================================
Coverage 97.27% 97.27%
=======================================
Files 126 127 +1
Lines 7674 7678 +4
=======================================
+ Hits 7465 7469 +4
Misses 209 209 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Krastanov
left a comment
Krastanov
left a comment
There was a problem hiding this comment.
Thanks for the reminder! We are a bit short staffed, so reminders like this are certainly appreciated and regrettably necessary (otherwise a lot of the volunteer reviewers just do not remember about a PR).
I have two main questions / suggestions before we continue with a more in-depth review.
- The stubs should not be VNGraphs specific. I.e. these happen to be algorithms that are defined in VNGraphs.jl, but there might be other sources for them as well. Thus, we should organize them in a way that fits the overall API and file structure in terms of semantics, not in terms of provenance of the implementation.
- There already seems to be a bit about coloring in Graphs.jl, so maybe we can organize these stubs appropriately around what is already present. There is for instance the function
greedy_colorand the structColoring. - There is also the library GraphsColoring.jl in this same organization that somewhat tries to follow the Graphs.jl API -- we need to decide whether the VNGraphs.jl hookup makes most sens in here or in GraphsColoring.jl. If it is in-here, it probably makes most sense to proceed with defining appropriate APIs and extending them. If it is in GraphsColoring.jl then once can do a pkgext in VNGraphs.jl for the already existing API in GraphsColoring.
@djukic14, could you chime in on your opinion on how to proceed. For context, Mahmud has been working on a few PRs in the ecosystem that are meant to provide a standard Graphs.jl API to wrappers around a number of other external libraries (lemon, very_naughty, igraph, etc).
| error( | ||
| "chromatic_number is not implemented in Graphs.jl. " * | ||
| "Please load VNGraphs.jl to use the very_nauty implementation.", | ||
| ) |
There was a problem hiding this comment.
I am not a big fan of this style of defining functions to be extended. This function will get invalidated when another library gets imported, causing bad user experience (latency for imports/precompilation). I think a cleaner way to do this is to use an error hint defined in the __init__ function. The WeakDepHelpers.jl library is trying to automate some of this.
|
@Krastanov @mahmudsudo I’d be happy to help integrate this into GraphsColoring.jl. I also think we should aim for a generic API (both here and in GraphsColoring.jl) so different implementations can plug in without being tied to a specific backend. |
|
@djukic14 , do you think the functions |
|
@Krastanov for now it probably makes sense for these to live in Graphs.jl, but we should discuss whether (in a future breaking change) all coloring-related APIs should move to GraphsColoring.jl for consistency, though I’m unsure how the community would feel about that. |
Krastanov
left a comment
Krastanov
left a comment
There was a problem hiding this comment.
Thanks, @djukic14 !
@mahmudsudo , given what was discussed, and given the existence of GraphsColoring.jl and GraphsOptim.jl with which we will need to coordinate in the future, how do you feel about the following (submitted as a new review so that the comments are grouped together, see comments)
Let's also start discussing how the bounty will be paid -- send me an email so that I can give you the details. I want to make sure you have them, as the payout processing from our nonprofit can be pretty slow (everyone is a volunteer here, as I have mentioned embarrassingly often :D )
There was a problem hiding this comment.
for now, let's keep the stubs private, so that it is not a breaking change when we decide how to incorporate them with GraphsOptim and GraphsColoring
i.e., we will not be including documentation pages and this one should be deleted
| chromatic_number, | ||
| edge_chromatic_number |
| Note: This is a stub provided by `Graphs.jl`. For a concrete implementation, | ||
| please use the `VNGraphs.jl` package which provides a fast C implementation | ||
| via `very_nauty`. |
There was a problem hiding this comment.
if you are including "notes" use the Documenter.jl note syntax that puts it in a separate panel
And let's make the API be chromatic_number(g, ::Algorithm) to clearly state that you need to specify an algorithm implementation. Mention VNGraphs.jl as one currently implemented option, but do not make it sound like it is the only one
There was a problem hiding this comment.
the timeout should be an argument of the Algorithm type to keep things clean (so no need to mention it here)
| ```julia | ||
| using VNGraphs | ||
| chromatic_number(g) |
There was a problem hiding this comment.
if you include code in docstrings, make sure it is a doctest, so that it is automatically checked in case it might be outdated. Here we can probably just remove it altogether
| function chromatic_number(g::AbstractGraph, args...; kwargs...) | ||
| error( | ||
| "chromatic_number is not implemented in Graphs.jl. " * | ||
| "Please load VNGraphs.jl to use the very_nauty implementation.", | ||
| ) | ||
| end |
There was a problem hiding this comment.
| function chromatic_number(g::AbstractGraph, args...; kwargs...) | |
| error( | |
| "chromatic_number is not implemented in Graphs.jl. " * | |
| "Please load VNGraphs.jl to use the very_nauty implementation.", | |
| ) | |
| end | |
| function chromatic_number end |
We want just to declare it as part of the namespace without giving it an implementation. No need to worry about adding error hints even, given that it is private for now
| """ | ||
| edge_chromatic_number(g, timeout=0) | ||
| Return the edge chromatic number of the graph `g`. | ||
| Note: This is a stub provided by `Graphs.jl`. For a concrete implementation, | ||
| please use the `VNGraphs.jl` package which provides a fast C implementation | ||
| via `very_nauty`. | ||
| ```julia | ||
| using VNGraphs | ||
| edge_chromatic_number(g) | ||
| ``` | ||
| """ | ||
| function edge_chromatic_number(g::AbstractGraph, args...; kwargs...) | ||
| error( | ||
| "edge_chromatic_number is not implemented in Graphs.jl. " * | ||
| "Please load VNGraphs.jl to use the very_nauty implementation.", | ||
| ) | ||
| end |
| """ | ||
| Graphs | ||
| include("interface.jl") | ||
| include("vngraphs_stubs.jl") |
There was a problem hiding this comment.
these stubs are not VNGraphs specific, rather they are for coloring. Maybe we can name the file coloring or see if there is already existing file about these concepts?
| "trees/prufer", | ||
| "experimental/experimental", | ||
| "planarity", | ||
| "vngraphs_stubs", |
There was a problem hiding this comment.
given that these are kept private, I guess there are no new tests to be added
|
|
||
| ## unreleased | ||
| - `is_articulation(g, v)` for checking whether a single vertex is an articulation point | ||
| - `chromatic_number` and `edge_chromatic_number` stubs (implemented in `VNGraphs.jl`) |
There was a problem hiding this comment.
call them declared private functions without any methods instead of stubs and point to VNGraphs.jl as one place that happens to implement methods, but do not make it sound like it is meant to be the only one
3 participants
Declares chromatic_number and edge_chromatic_number in Graphs.jl with helpful error hints directing users to install VNGraphs.jl for the fast C-backed very_nauty implementations.
Includes unit tests asserting the stubs throw the expected errors.
VNGraph pr : JuliaGraphs/VNGraphs.jl#26
closes #448