feat: change "incidence matrix" naming to "biadjacency matrix" #962
Conversation
Current Aviator status
This PR was merged using Aviator. See the real-time status of this PR on the Aviator webapp. Use the Aviator Chrome Extension to see the status of your PR within GitHub.
|
|
This becomes a small blocker for #716 since it edits the zzz-deprecate.R script. |
R/conversion.R
Outdated
| #' @keywords internal | ||
| #' @export | ||
| as_incidence_matrix <- function(...) { | ||
| lifecycle::deprecate_soft("1.5.2", "as_incidence_matrix()", "as_biadjacency_matrix()") |
There was a problem hiding this comment.
@krlmlr what version number should we use here?
There was a problem hiding this comment.
Can you please update the documentation text to refer to "bipartite adjacency matrix" instead of "incidence matrix" in all places? Add just one note along these lines:
Some authors refer to the bipartite adjacency matrix as the "bipartite incidence matrix". igraph 1.6.0 and later does not use this naming to avoid confusion with the edge-vertex incidence matrix.
|
This pull request failed to merge: this PR has a review with changes requested (the review must be approved or dismissed before merging). Remove the |
|
@szhorvat: Please remove the "blocked" label when good. |
|
@krlmlr Looks good to me, but are you going to do 1.5.2 instead of 1.6.0? There are a lot of changes here, worth a version bump ... |
|
Why 1.5.2? |
|
The version numbers in this PR are all 1.5.2, and @maelle was asking above about what version number you were going to use. See her comment on I think 1.6.0 makes more sense, but it's your decision. Whatever the decision, we should be consistent about it in the docs. |
|
@krlmlr This looks fine for me, other than the version. I'll leave it to you to deal with what version is mentioned in the docs. |
|
This pull request failed to merge: PR cannot be automatically rebased, please rebase manually to continue. Remove the Additional debug info: Failed to rebase this PR onto the latest changes from the base branch. You will probably need to rebase this PR manually and resolve conflicts). |
|
This pull request failed to merge: PR cannot be automatically rebased, please rebase manually to continue. Remove the Additional debug info: Failed to rebase this PR onto the latest changes from the base branch. You will probably need to rebase this PR manually and resolve conflicts). |
krlmlr
changed the title
|
Thanks! This PR doesn't seem to be breaking anything. |
|
@maelle: Can you please work on the test coverage? Probably it's sufficient to test that the deprecated variants give the same results as the original, plus a deprecation warning? |
|
This pull request can't be queued because it's currently a draft. |
|
Thanks! |
Fix #900