rigraph icon indicating copy to clipboard operation
rigraph copied to clipboard

fix: `biconnected_components()` returns invalid result fields

Open Copilot opened this issue 4 months ago • 0 comments

The auto-generated biconnected_components_impl() was creating duplicate empty fields with dot notation (tree.edges, component.edges, articulation.points) alongside the correct underscore fields from the C layer.

Root Cause

The impl function received underscore field names from C but incorrectly accessed them using dot notation:

res$tree.edges <- lapply(res$tree.edges, ...)  # res$tree.edges doesn't exist yet

This created new empty dot fields while leaving the original underscore fields unchanged, resulting in:

> names(res)
[1] "no" "tree_edges" "component_edges" "components" "articulation_points"
[6] "tree.edges" "component.edges" "articulation.points"  # Empty duplicates

Changes

  • R/aaa-auto.R: Fixed field access to use underscore notation (res$tree_edges, res$component_edges, res$articulation_points)
  • R/components.R: Removed redundant workaround that was recreating underscore fields and deleting empty dot fields
  • tests/testthat/_snaps/aaa-auto.md: Updated snapshot to reflect correct output format

Result

> res <- biconnected_components(make_graph(c(1,2, 2,3, 3,1, 1,4), directed = FALSE))
> names(res)
[1] "no" "tree_edges" "component_edges" "components" "articulation_points"

All fields now contain properly typed igraph.vs/igraph.es objects.

Original prompt

This section details on the original issue you should resolve

<issue_title>biconnected_components() returns invalid result fields</issue_title> <issue_description>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
}

</issue_description>

Comments on the Issue (you are @copilot in this section)

@ntamas 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. @szhorvat 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().</comment_new> <comment_new>@szhorvat

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.</comment_new> <comment_new>@ntamas

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.</comment_new> <comment_new>@szhorvat

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.</comment_new> <comment_new>@krlmlr

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...

  • Fixes igraph/rigraph#1203

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot avatar Oct 26 '25 18:10 Copilot