fix: Return parent as numeric vector in bfs() and dfs() instead of igraph.vs
#2455
+59
−11
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
bfs()anddfs()functions returnedparentas anigraph.vsobject containing NA values for root vertices, causingstr()and indexing operations to fail with "Unknown vertex selected" errors.Changes
R/structural-properties.R:
parenttoigraph.vsin bothbfs()anddfs()tests/testthat/test-structural-properties.R:
parentis numeric vector with proper NA handlingstr()and indexing operations work correctlytests/testthat/_snaps/structural-properties.md:
The
parentresult is now documented correctly as "Numeric vector" and enables use cases like Strahler number computation that require standard R vector operations.Fixes #1576
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
cran.rstudio.com/opt/R/4.5.1/lib/R/bin/exec/R --no-restore --no-echo --args nextArgigraph_2.2.1.9002.tar.gznextArg--as-crannextArg--timingsnextArg--no-manual(dns block)packagemanager.posit.co/opt/R/4.5.1/lib/R/bin/exec/R --no-restore --no-echo --args nextArgigraph_2.2.1.9002.tar.gznextArg--as-crannextArg--timingsnextArg--no-manual(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
This section details on the original issue you should resolve
<issue_title>bfs()/dfs() adds NA into 'father' vector, which then causes trouble later</issue_title>
<issue_description>Describe the bug
The
bfs()anddfs()functions produce a 'father' vector that containsNAfor the root. This is in fact not a vector, but anigraph.vs, where it is reasonable to expect that all values are proper vertex IDs and notNA. As a result, there will be problems when trying to use this result.This is closely related to #186. Perhaps the same fix should be applied as there. While this will be a breaking change, the
fatherresults appears to be unusable at the moment, as theNAs are basically guaranteed to trigger an error (unless I'm missing something about how to work withNA).To reproduce
I am not sure how to even test whether a certain element or
NA, which is what originally prevented me from being able to come up with a nice solution for the Strahler number computation here.Version information
1.3.0, but also earlier.
</issue_description>
Comments on the Issue (you are @copilot in this section)
@szhorvat Ultimately, the question is how to represent trees through parent vectors, see https://github.com/igraph/igraph/issues/1880 Also, what to call parent vectors? `parent`? `father`? `predecessor`? @ntamas I'm going to keep `father` for the 1.x series in order not to break the API; we can reconsider for 2.0 where we will need to break all sorts of things anyway.As for
NA,res[1, na_ok=T]works now after d88657c. I'll look into enabling NA values forstr(res).</comment_new><comment_new>@ntamas
Okay, so supporting
str(res)is not easy. In the end, it boils down to a call to[.igraph.vsfrom an internal part of the R code that I have no control over. So, either we make it possible forigraph.vsto remember whether it was constructed withna_okin the first place so sub-slices created from it can "inherit" this property, or we makena_ok=Tthe default, which is a breaking change.</comment_new><comment_new>@ntamas
Postponing the rest of this issue to 2.x; indexing now works with the
na_ok=...trick outlined above, and supportingstr(res)would require deeper changes that we are not prepared to do now.</comment_new><comment_new>@krlmlr
str()is a generic, can we implement a method for it?We could mention
na_okin the error message.</comment_new><comment_new>@krlmlr
As for the presence of
NAvalues, I don't see it as a big problem. This is idiomatic R.</comment_new><comment_new>@ntamas
I didn't know that
stris a generic at the time I wrote it. If we can work around the issue with that, that's great, but I don't think it's a priority. Also, ifNAvalues in a result vector like this is idiomatic R and people know who to do with it, that's OK and we can essentially close this issue.</comment_new><comment_new>@szhorvat
Note that this result is not a vector, but a vertex sequence, i.e.
igraph.vs.I think this is problematic:
igraph.vshaving NAs. On the C side, anigraph_vs_t, which anigraph.vswould typically map to, cannot have missing values. To make things worse, R arbitrarily designates a particular integer as the "NA value", but on the C side, this will appears as a normal integer. We don't want to have to know about R's NA weirdness in the C library. Starting with igraph 0.10, vertex indices are proper integers.igraph.vsat all. What we have instead is a vertex -> vertex mapping. And yes, some vertices do not map to anything, we need to represent that somehow (with an NA or otherwise). To allow that, isn't it better to use a different result type thanigraph.vsinstead of trying to tolerate NAs in vertex sequences?@krlmlr Before making up your mind on this, I strongly suggest you implement a Strahler index computation using
bfs()to experience the issues first-hand.As I said above, the first issue is whether the result should be an
igraph.vsat all, give that there is a need for representing missing values. There is ...💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.