igraph
`biconnected_components()` returns invalid result fields
biconnected_components() returns a result list with named components tree_edges, component_edges, articulation_points. These list components have valid values, However, the result also has components tree.edges, component.edges and articulation.points. These are empty.
Note that biconnected_components() is fully auto-generated, but somewhere during auto-generation the _ is changed to a ., but only in some places, not everywhere.
> res<-biconnected_components(make_graph(c(1,2, 2,3, 3,1, 1,4), directed = F))
> res
$no
[1] 2
$tree_edges
$tree_edges[[1]]
[1] 2 1
$tree_edges[[2]]
[1] 4
$component_edges
$component_edges[[1]]
[1] 3 2 1
$component_edges[[2]]
[1] 4
$components
$components[[1]]
+ 3/4 vertices, from c488bbb:
[1] 3 2 1
$components[[2]]
+ 2/4 vertices, from c488bbb:
[1] 4 1
$articulation_points
[1] 1
$articulation.points
+ 0/4 vertices, from c488bbb:
Also notice the function definition:
biconnected_components_impl <- function(graph) {
# Argument checks
ensure_igraph(graph)
on.exit( .Call(R_igraph_finalizer) )
# Function call
res <- .Call(R_igraph_biconnected_components, graph)
if (igraph_opt("return.vs.es")) {
res$tree.edges <- lapply(res$tree.edges, unsafe_create_es, graph = graph, es = E(graph))
}
if (igraph_opt("return.vs.es")) {
res$component.edges <- lapply(res$component.edges, unsafe_create_es, graph = graph, es = E(graph))
}
if (igraph_opt("return.vs.es")) {
res$components <- lapply(res$components, unsafe_create_vs, graph = graph, verts = V(graph))
}
if (igraph_opt("return.vs.es")) {
res$articulation.points <- create_vs(graph, res$articulation.points)
}
res
}
The replacement is done in Stimulus in src/stimulus/generators/r.py in the function named get_r_parameter_name -- that part of the code is actually replacing underscores with dots. I don't have more time to debug this, but start looking there.
This broke between 1.2.11 and 1.3.0. Isn't that when stimulus was separated into a library?
This specific function used underscore names in 1.2.11, res$articulation_points
The autogenerated code in 1.3.0 uses a buggy mixture of underscore and dot, and ends up with the mess I showed above. Furthermore, the outputs are no longer a proper vertex/edges sequences.
As Tamás said in chat, we need to decide whether we go with dots or underscores for output parameter names. Input parameters and attributes already use dots, so that's a natural choice, but that would involve a breaking change for biconnected_components().
I would have expected this to be fixable using
igraph_biconnected_components:
PARAM_NAMES:
articulation_points: articulation_points
tree_edges: tree_edges
component_edges: component_edges
but this doesn't disable the _ -> . replacement. If the parameters are renamed to anything but the original name, then the replacement is disabled. It would be good to fix this in Stimulus, to allow for special cases when we DON'T want the .-renaming.
The _ -> . replacement is specific to the R code generator and it was there to enforce R's then-preferred dotted notation without having to spell out the alternative names one by one for each argument. Also, this came before we've had PARAM_NAMES. I wouldn't try to fix the treatment of PARAM_NAMES vs the replacement, I'd rather prefer first to decide once and for all which of the two formats (dotted or underscored) is preferred in the R interface. If it is the dotted notation, then everything is fine and we don't need any special treatment in PARAM_NAMES. If it is the underscored notation, we need to remove the replacement code and that's all.
I'd rather prefer first to decide once and for all which of the two formats (dotted or underscored) is preferred in the R interface. If it is the dotted notation, then everything is fine and we don't need any special treatment in
PARAM_NAMES. If it is the underscored notation, we need to remove the replacement code and that's all.
@krlmlr, the ball's in your court on this one.
Changing the component names in a back-compatible way seems difficult, I wonder if it's worth the effort.
Just breaking the functionality seems to require a lot of work with downstream maintainers, I wouldn't go that route.
We could use a list that has both new and old names, but prints only the new names. Ideally, we'd also give a lifecycle warning when touching the old names. We might need even more flexibility in Stimulus to support this.
