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 as_adjacency_matrix() in test-conversion #1519

Merged
merged 6 commits into from
Sep 30, 2024

Conversation

maelle
Copy link
Contributor

@maelle maelle commented Sep 23, 2024

@szhorvat this is to prep for part of #1518, prior to my PR there were no direct tests of as_adjacency_matrix().

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 manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.


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 test-as_adjacency_matrix branch from 87037d9 to 880698e Compare September 23, 2024 09:05
@maelle maelle force-pushed the test-as_adjacency_matrix branch from 880698e to 33e1558 Compare September 23, 2024 09:18
@maelle maelle marked this pull request as ready for review September 23, 2024 09:18
@maelle maelle requested a review from szhorvat September 23, 2024 09:18
@maelle maelle added the upkeep maintenance, infrastructure, and similar label Sep 23, 2024
@szhorvat
Copy link
Member

You are using randomly generated data, then testing for specific values that depend on the random seed. This is difficult to manage and it's hard to see what goes wrong with failures.

I suggest:

  • Create a small specific non-random graph, which has self-loops and multi-edges. 3 or 4 vertices should suffice.
  • Create non-random edge weights. I suggest that these should be simple non-integers rounded to a single digit after the point (e.g. 1.2, 0.1, 2.0, etc.), for readability.
  • Test the whole matrix for exact equality. Do not just test specific elements. Do not test row or column sums for a specific value. Once the loops argument is sorted out, we can test that row/column sums equal the out/in degree (unweighted) and strength (weighted). That is not true right now, so don't test it yet.
  • Ideally, test this both for a directed and undirected instance.

This would make a complete test.

It is important to test self-loops and multi-edges, which is where things go wrong!

@szhorvat
Copy link
Member

szhorvat commented Sep 24, 2024

For example,

dg <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed=T)
ug <- as.undirected(g, mode="each")

The directed version contains: multi-edges, reciprocal edges, self-loops, multi-self-loops. The undirected version has the same edges, with directions ignored. I chose a 4-vertex graph (instead of a 3-vertex one) to leave room for non-diagonal zeros in the undirected case, and better cover all cases.

Then just add edge weights.

@maelle
Copy link
Contributor Author

maelle commented Sep 24, 2024

Thank you!!

@maelle
Copy link
Contributor Author

maelle commented Sep 24, 2024

@szhorvat I've implemented the changes, is this good to go?

@szhorvat
Copy link
Member

Based on a quick skim, yes!

Copy link
Contributor

aviator-app bot commented Sep 30, 2024

This pull request failed to merge: PR cannot be automatically rebased, please rebase manually to continue. After you have resolved the problem, you should remove the blocked pull request label from this PR and then try to re-queue the PR. Note that the pull request will be automatically re-queued if it has the mergequeue label.

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).

@maelle maelle merged commit d56366c into main Sep 30, 2024
11 of 12 checks passed
@maelle maelle deleted the test-as_adjacency_matrix branch September 30, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked upkeep maintenance, infrastructure, and similar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants