feat: Any all-NA weights means unweighted, and announce the weights = NULL default change - #2847
Open
krlmlr wants to merge 2 commits into
Open
feat: Any all-NA weights means unweighted, and announce the weights = NULL default change#2847krlmlr wants to merge 2 commits into
weights means unweighted, and announce the weights = NULL default change#2847krlmlr wants to merge 2 commits into
Conversation
…idance #2677 made `weights = NULL` pick up the `weight` edge attribute in the adjacency matrix functions and in `power_centrality()`, which brings them in line with `distances()` and the rest of igraph. It is intentional and consistency is worth it, but it changes what those functions return for any graph carrying a `weight` attribute, silently and with no warning, and NEWS had it only as a Features line about retiring `attr`. The entry names every affected function, shows the before and after, and gives the migration: `weights = NA`. `power_centrality()` gets called out separately. It had no weight argument at all before #2677, so for it this is a new default rather than a renamed one. It also says what *not* to write. `weights = numeric()` gets the old behaviour from `as_adjacency_matrix()` -- `all(is.na(numeric()))` is `TRUE` -- but it is not a supported spelling, and `distances()` rejects it against `ecount()`. Recommending it would hand people something that works in one function and errors in the next. Neither spelling exists on 2.3.3, where these functions took `attr`, so the entry gives two expressions that work on both versions for code that has to span them. This edits a fledge-managed file by hand, which the header asks contributors not to do. A breaking change that needs a migration path does not fit in a generated one-line bullet, and fledge prepends new versions rather than rewriting old ones, so the section survives the next bump. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D1xpHRV7yVfgtJg4vp9P7z
`distances()` and `shortest_paths()` tested `length(weights) == 1 && is.na(weights)`, so a single `NA` meant unweighted but `rep(NA, ecount(g))` and `numeric()` were errors -- while meaning "unweighted" in every generated wrapper in `R/aaa-*.R`, which have always tested `all(is.na(weights))`. Both now test `all(is.na(weights))`. `all(is.na(numeric()))` is `TRUE`, so the empty vector comes along with it, and the rule is one sentence: an all-NA `weights` is unweighted. A vector in which only *some* entries are NA is still rejected, by the C core, which reports `Weights must not contain NaN values` -- R's `NA_real_` reaches C as a NaN, so `NA` and `NaN` are the same thing there. A vector of the wrong length is still the wrong length. The NEWS entry from the previous commit now says "all NA" rather than naming the single `NA`, and no longer warns people off `numeric()`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D1xpHRV7yVfgtJg4vp9P7z
weights = NULL default change, with migration guidanceweights means unweighted, and announce the weights = NULL default change
Contributor
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e608670 is merged into main:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Two things, because the first is the answer to "which spelling do we recommend".
1. An all-NA
weightsmeans unweighted, everywheredistances()andshortest_paths()testedlength(weights) == 1 && is.na(weights), so a singleNAmeant unweighted butrep(NA, ecount(g))andnumeric()were errors — while meaning "unweighted" in every generated wrapper inR/aaa-*.R, which have always testedall(is.na(weights)). Two hand-written sites, now testing the same thing.all(is.na(numeric()))isTRUE, so the empty vector comes along with it, and the rule is one sentence: an all-NAweightsis unweighted.What the C core does with NaN
You asked. Partial NaN is rejected, and that is unchanged here:
R's
NA_real_reaches C as a NaN payload, soNAandNaNare indistinguishable down there — which is why one error message covers both, and whyweights = NaNalready meant "unweighted" viaall(is.na()). A wrong-length vector is still a wrong-length vector.One inconsistency this does not address:
as_adjacency_matrix(g, weights = c(1, NaN, 1, 1))silently returns a matrix withNaNentries, wheredistances()errors. Worth a separate look — say the word.2. The NEWS entry
#2677 made
weights = NULLpick up theweightedge attribute in the adjacency matrix functions and inpower_centrality(). Intentional, and consistency is worth it, but it changes what those functions return for any graph carrying aweightattribute, silently, and NEWS had it only as a Features line about retiringattr.The entry names every affected function —
as_adjacency_matrix()/as_adj()/get.adjacency(),as_biadjacency_matrix()/as_incidence_matrix()/get.incidence(),power_centrality()/bonpow()— notes thatalpha_centrality()is unchanged because it already readweight, and calls outpower_centrality()separately: it had no weight argument at all before, so for it this is a new default rather than a renamed one.The migration is spelled as "an all-NA
weights", coveringNA,rep(NA, ecount(g))andnumeric()in one rule, with the note that a partly NA vector is still an error. For code that has to span 2.3.3, where none of these exist, it givesas_adjacency_matrix(delete_edge_attr(g, "weight"))and(as_adjacency_matrix(g) != 0) * 1.Caveat
This edits
NEWS.mdby hand, which its fledge header asks contributors not to do. A breaking change that needs a migration path does not fit in a generated one-line bullet, and fledge prepends new version sections rather than rewriting old ones, so this survives the next bump. An article undervignettes/articles/is the alternative if you would rather.Every snippet in the entry was executed.
test-structural-properties,test-adjacency,test-conversionandtest-centralityare green.🤖 Generated with Claude Code
https://claude.ai/code/session_01D1xpHRV7yVfgtJg4vp9P7z