Skip to content

test: add tests for graph_from_biadjacency_matrix()#1520

Merged
aviator-app[bot] merged 6 commits intomainfrom
graph.incidence.dense-test
Oct 1, 2024
Merged

test: add tests for graph_from_biadjacency_matrix()#1520
aviator-app[bot] merged 6 commits intomainfrom
graph.incidence.dense-test

Conversation

@maelle
Copy link
Copy Markdown
Contributor

@maelle maelle commented Sep 23, 2024

Also characterization tests as prep for #1518

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app Bot commented Sep 23, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

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.

@maelle maelle force-pushed the graph.incidence.dense-test branch 2 times, most recently from e16ab0a to 0c7968a Compare September 23, 2024 12:55
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Sep 23, 2024

Based on the coverage results https://app.codecov.io/gh/igraph/rigraph/pull/1520/blob/R/incidence.R

I need to add weights to the cases that I thought would cover the modes for graph.incidence.sparse().

For graph.incidence.dense()... I am confused why the different modes aren't covered, I'll have to step into the function.

@maelle maelle force-pushed the graph.incidence.dense-test branch from 0c7968a to 7f7b02c Compare September 24, 2024 10:23
@maelle maelle marked this pull request as ready for review September 24, 2024 10:23
@maelle maelle requested a review from szhorvat September 24, 2024 10:23
Comment thread tests/testthat/test-incidence.R
@maelle maelle force-pushed the graph.incidence.dense-test branch from 7f7b02c to 5aafbf8 Compare September 30, 2024 12:38
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Sep 30, 2024

@szhorvat do these characterization tests look ok to you? (needed for #1518)

Comment thread tests/testthat/test-incidence.R Outdated
Comment thread tests/testthat/test-incidence.R Outdated
Comment thread tests/testthat/test-incidence.R Outdated
Comment thread tests/testthat/test-incidence.R Outdated
Comment thread tests/testthat/test-incidence.R Outdated
Comment thread tests/testthat/test-incidence.R Outdated
@szhorvat
Copy link
Copy Markdown
Member

szhorvat commented Oct 1, 2024

weighted=T and multiple=T should be mutually exclusive. We either interpret numbers larger than 1 as weights or as multiplicities, but it cannot be both.

I suggest adding an error when setting both to true.

This is not an issue in the C core, as we have separate functions for the unweighted and weighted cases, and only the unweighted function has a multiple parameter. But in R we have a single function (and we should keep it that way).

Comment thread tests/testthat/test-incidence.R
local_igraph_options(print.id = FALSE)
withr::local_seed(42)

inc <- matrix(sample(0:1, 15, repl = TRUE), 3, 5)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment about weights as before. Have a diversity of weights. Have at least two different kinds of weights (sample(0:2)), but ideally have non-integer weights as well. It's a question of how much time you want to dedicate to testing. There are lots of cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@aviator-app aviator-app Bot force-pushed the graph.incidence.dense-test branch from c1f1133 to be6845b Compare October 1, 2024 13:33
@aviator-app aviator-app Bot merged commit 71aa0b5 into main Oct 1, 2024
@aviator-app aviator-app Bot deleted the graph.incidence.dense-test branch October 1, 2024 14:35
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants