-
-
Notifications
You must be signed in to change notification settings - Fork 203
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
feat: get_edge_ids()
accepts data frames and matrices
#1663
Conversation
Current Aviator status
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.
|
Line 186 in fe3b5b8
The case of a 2x2 matrix will be tough to properly handle because we dont know if the user suplied edges row or column wise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The warnings in tests are all from "wrong" use of cc @maelle If you have time (no rush) can you check if my use of lifecycle (in code and tests) is ok? (never actively used it before) hmm ok, I get warnings locally due to the deprecation of 2 x n matrices. Seems not to be checked here? cc @krlmlr |
Co-authored-by: Kirill Müller <[email protected]>
Co-authored-by: Kirill Müller <[email protected]>
769703c
to
547fca1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, can you please tweak and auto-merge when good?
Co-authored-by: schochastics <[email protected]>
Also for
add_edges()
?The matrix case works already by accident, a two-row matrix will give the intended result. Should we soft-deprecate the matrix case to later support two-column matrices?
Created on 2025-01-20 with reprex v2.1.1