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

refactor: forward get.adjacency.dense() to get.adjacency.sparse() if attributes are present (#1483) #1653

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

schochastics
Copy link
Contributor

This PR removes (nested) for loops for get.adjacency.dense() when attr != NULL

(cherry picked commits from #1518)

Copy link
Contributor

aviator-app bot commented Jan 9, 2025

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 pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


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.

@schochastics schochastics changed the title removed nested for loops in get.adjacency.dense (#1483) refactor: removed nested for loops in get.adjacency.dense (#1483) Jan 9, 2025
@schochastics
Copy link
Contributor Author

Test fails because tests in main are currently not correct, see #1551 (and also #1518). Should the tests be fixed here?

cc @maelle @krlmlr

@krlmlr
Copy link
Contributor

krlmlr commented Jan 13, 2025

Every PR should be green, yes. If this needs changing tests, it can be done in the same PR.

The best would be to change tests first, in a separate commit or PR, so that they work both with the existing and the new implementation. Not sure if it's worth the effort.

Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, great! Somehow dimnames() are lost in R 4.1 and earlier, the checks are failing.

tests/testthat/_snaps/conversion.md Outdated Show resolved Hide resolved
@krlmlr krlmlr changed the title refactor: removed nested for loops in get.adjacency.dense (#1483) refactor: forward get.adjacency.dense() to get.adjacency.sparse() if attributes are present (#1483) Jan 14, 2025
@schochastics
Copy link
Contributor Author

Ready to be reviewed again

Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, we're close.

R/conversion.R Outdated Show resolved Hide resolved
R/conversion.R Outdated Show resolved Hide resolved
@schochastics
Copy link
Contributor Author

ready again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants