Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add tests for graph_from_biadjacency_matrix() #1520

Merged
merged 6 commits into from
Oct 1, 2024

Conversation

maelle
Copy link
Contributor

@maelle maelle commented Sep 23, 2024

Also characterization tests as prep for #1518

Copy link
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
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 marked this pull request as ready for review September 24, 2024 10:23
@maelle
Copy link
Contributor Author

maelle commented Sep 30, 2024

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

@szhorvat
Copy link
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).

local_igraph_options(print.id = FALSE)
withr::local_seed(42)

inc <- matrix(sample(0:1, 15, repl = TRUE), 3, 5)
Copy link
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
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
11 checks passed
@aviator-app aviator-app bot deleted the graph.incidence.dense-test branch October 1, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants