-
Notifications
You must be signed in to change notification settings - Fork 109
Add stubs #456
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
base: master
Are you sure you want to change the base?
Add stubs #456
Conversation
I closed the related PR in igraphs as it had some structural issues and I would prefer to go with smaller PRs first. This looks very promising here, thanks for submitting it! There are some idioms that are a bit strange, but we should be able to fix them together. I will leave a few comments inline. |
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.
This is a great start! Here are a few suggestions that can hopefully simplify your work.
|
||
```julia | ||
using Graphs, IGraphs | ||
using IGraphs.GraphsCompat # enable Graphs.jl interface for igraph |
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.
I do not believe that a separate interface module is necessary. Julia is a multiple dispatch language. You just need to add a new method to the existing functions, nothing more. We already have an IGraph type (that might need to be improved), but new graph types should not be needed.
@@ -463,7 +463,7 @@ include("interface.jl") | |||
include("utils.jl") | |||
include("deprecations.jl") | |||
include("core.jl") | |||
|
|||
include("igraph_stubs.jl") |
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.
these should not be igraph specific. There are many other libraries that implement interesting algorithms and we are implementing extensions for them. If an algorithm does not exist in Graphs, a function should be declared in the appropriate location for that algorithm (without any methods attached to that function).
E.g. if we want coreness
, the function declaration should be added to the appropriate place for the concept of coreness
, not to an igraph specific file, as another library might also implement it.
Let's not export any of these though, so they are not available by default.
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.
on further consideration, I see the point of organizing things the way you did, with everything in a single file if it does not have implementation outside of igraph.
Maybe we can structure it in a simpler way that automates how the hints are added too. Something like this:
"""
These are algorithms available in external wrapper libraries (like `igraph`/`LEMON`/`networkx`) but not available in any Julia-native libraries in the JuliaGraphs ecosystem.
They are all listed in this single dictionary as a convenient way to automatically add nice error hints directing users to the appropriate wrapper library.
We are encouraging Julia-native implementations and would be happy to help any new contributors implement Julia versions of these algorithms -- simply go ahead and submit a PR to Graphs.jl.
When such a contribution is made, we can remove the contributed algorithm from this dict.
"""
const EXTERNAL_STUBS = Dict(
:function_name => (
docstring=""" yada yada yada
yada yada
""",
libs=[:IGraphs]
),
...
)
And then we can just have an eval that declares the needed functions.
""" | ||
function assortativity_nominal end | ||
assortativity_nominal(g::AbstractGraph, attr; kwargs...) = | ||
throw(ArgumentError(_igraph_backend_msg(:assortativity_nominal))) |
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.
This probably should just throw the default MethodError, not have a custom error defined for each method. A big drawback of the current approach you have here is the significant extra compilation and then invalidation on importing other packages.
In julia it is idiomatic to rely on MethodErrors to mark unimplemented APIs. Then you can add error hints to be more helpful to the user. All of these should probably be converted to error hints, but let's leave that to another PR -- for the moment we can just focus on removing all these default methods.
# ----------------------- | ||
# Coreness / k-core number | ||
# ----------------------- | ||
@static if !isdefined(@__MODULE__, :coreness) |
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.
I do not understand the point of these if isdefined
.
This is the forth part of #446 which require some work to be in this repo:
'If there is an algorithm defined in igraph that does not exist yet in Graphs.jl, it should be declared in Graphs.jl (just a function with docs but no methods), together with an error hint that IGraphs is necessary. Given the large scope of the library, this can be more of a proof-of-concept setup, documenting how to add everything, but not expecting everything to be added for completing the bounty.'