Skip to content

Conversation

schochastics
Copy link
Contributor

@schochastics schochastics commented Jul 3, 2025

Fix #46 (check if #60 could be addressed)

Should this be an error now?

rigraph/R/operators.R

Lines 264 to 266 in 0386a24

if ("name" %in% names(attr) && any(duplicated(attr$name))) {
cli::cli_warn("Duplicate vertex names in disjoint union.")
}

also need a strategy for this

library(igraph)
g1 <- g2 <- make_ring(4)
V(g1)$name <- c("A", "B", "C", "D")
V(disjoint_union(g1, g2))$name
#> Warning: Duplicate vertex names in disjoint union.
#> [1] "A" "B" "C" "D" NA  NA  NA  NA

Created on 2025-07-03 with reprex v2.1.1

@schochastics
Copy link
Contributor Author

re #60 and the disjoint_union() case:
For consistency, it might be safer to enforce consistent naming too. So we should probably also be throwing an error.

Cons: Might be annoying for quick prototyping

@schochastics
Copy link
Contributor Author

Should also look at #1250

R/attributes.R Outdated
if (is.null(current_names)) {
current_names <- rep(NA_character_, vcount(graph))
}
idx_numeric <- as.numeric(index)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? (genuine question)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the graph has no name attribute but we add a vertex with a name, we need to set all other names to NA (this is consistent with most of igraph but not really a good solution. Needs to be addressed elsewhere)

h <- make_full_graph(3)
set_vertex_attr(h, "name", 2, "B")
#>IGRAPH 4d3e0a7 UN-- 3 3 -- Full graph
#>+ attr: name (g/c), loops (g/l), name (v/c)
#>+ edges from 4d3e0a7 (vertex names):
#>[1] NA--B  NA--NA B --NA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was more curious about the as.numeric().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry. That was actually not needed and my mistake

Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve conversations, happy to review in a second iteration. The code looks complex.

@schochastics schochastics added this to the 2.3.0 milestone Jul 15, 2025
@schochastics
Copy link
Contributor Author

all comments addressed and I think I made it a bit simpler

@szhorvat
Copy link
Member

Does this affect only disjoint_union? There are other ways to create duplicate names.

@schochastics
Copy link
Contributor Author

schochastics commented Jul 16, 2025

Yes there should be more places. The question is, what should happen in this case. I think we reached a form of consensus last week, but I can't remember what we thought was the best way to deal with it. I think it might have been to throw an error if a named and unnamed object are combined

@schochastics
Copy link
Contributor Author

Ok I realise I talked about combining unnamed and named objects only. The pr is about erroring for duplicated vertex names. I think this covers all places where this can happen or do I see any other place?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

R: bug: adding vertices should check for duplicate vertex names

4 participants