-
-
Notifications
You must be signed in to change notification settings - Fork 206
Standardize the handling of optional vertex parameters #1572
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
Comments
On the R side, having |
I agree that the default on the interface should be @Antonov548: Can you please help with Stimulus? Are the requirements clear? |
Sorry, I'm not follow anymore. From the all pull requests and discussion I have thoughts that changes to Stimulus are not required. Can you poine me please what should be updated? |
Since we're going with option 2, I think you're right. But to be sure, it'd be great to actually implement this (by removing the default of 0 from |
The handling, and default value, of optional vertex parameters should be standardized, and Stimulus should be updated accordingly.
There are currently two C functions which take an optional vertex parameter:
igraph_random_spanning_tree()
andigraph_fundamental_cycles()
. Only the former is exposed at the moment. We need a decision on this issue before I expose the latter.In C, these two functions interpret a negative vertex index as not passing a vertex.
In R, the situation is more complicated because vertices can be referred to either by a string name or by an integer index. Both must be supported.
IMO there are two reasonable options:
NULL
is not allowed, nor are passing multiple vertices.NULL
. The default parameter value should beNULL
. Non-positive integers are not allowed. Multiple vertices are not allowed.We currently have (1) mostly implemented. The auto-generated code looks like this:
However, currently,
OPTIONAL VERTEX
, without an explicit default, usesNULL
as the default, which triggers the "No vertex was specified" error. If we go with choice (1), Stimulus should be updated to use0
instead ofNULL
as the default. If we go with choice (2), the autogen code template must be updated for this.@krlmlr, can you make a decision on this? When that is done, can you adapt Stimulus, @Antonov548 ?
The text was updated successfully, but these errors were encountered: