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

PyDiGraph: get ingoing edge indices of a node #1360

Open
hakkape opened this issue Jan 10, 2025 · 9 comments
Open

PyDiGraph: get ingoing edge indices of a node #1360

hakkape opened this issue Jan 10, 2025 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@hakkape
Copy link

hakkape commented Jan 10, 2025

What is the expected enhancement?

Similar to incident_edges() which can return the IDs of edges going out OR going in and out of a node in a directed graph, i would like to have a function to obtain the IDs of just the ingoing edges of a node. There is in_edges(), but this only returns tuples of (head_node, tail_node, data), so i would need to call another function to get the ID of each edge.

@IvanIsCoding
Copy link
Collaborator

IvanIsCoding commented Jan 10, 2025

I assume we could use a in_edge_indices function. That seems specific but reasonable

@IvanIsCoding IvanIsCoding added good first issue Good for newcomers enhancement New feature or request labels Jan 10, 2025
@IvanIsCoding IvanIsCoding added this to the 0.17.0 milestone Jan 15, 2025
@ThisuraGallage
Copy link

ThisuraGallage commented Jan 17, 2025

Hi... looks like this issue is free for grabs. May I take this up?

Edit: Just a bit confused about the proposed incident_edge_indices function. As far as I understand, the incident_edges() function already gives us indices for edges going out or both in and out (all incident edges). What seems to be missing is a function that would return only the indices of ingoing edges. So, I was wondering, what would the incident_edge_indices function return? Could you help me clarify this? Apologies if I’ve misunderstood anything!

@IvanIsCoding
Copy link
Collaborator

I will post a comment with the details later but yes this issue is available as a good first issue

@ThisuraGallage
Copy link

Great, I'll take on the issue!

@ThisuraGallage
Copy link

ThisuraGallage commented Jan 20, 2025

Hi,
I went through the code and read the contribution guidelines in CONTRIBUTING.md. I believe these are the places I need to make the changes, right? Apologies if I’m misunderstanding anything—kinda new to this!

Add the new function in, https://github.com/Qiskit/rustworkx/blob/main/src/digraph.rs
Add tests in, https://github.com/Qiskit/rustworkx/blob/main/tests/digraph/test_edges.py
Function signature in, https://github.com/Qiskit/rustworkx/blob/main/rustworkx/rustworkx.pyi
and a release note

@IvanIsCoding
Copy link
Collaborator

Yes, the places you outlined are correct! Sorry I haven’t had time to update the details yet but what inferred from Contributing.md should be enough to get you started.

I will try to add the details when I can and/or review your PR if you finish before I update

@ThisuraGallage
Copy link

ThisuraGallage commented Jan 20, 2025

No worries! I've created a draft PR, Please review it at your convenience and let me know your thoughts. I'm not entirely sure about the function signature as well. I'll add documentation once everything is finalized, thanks !

@IvanIsCoding
Copy link
Collaborator

No worries! I've created a draft PR, Please review it at your convenience and let me know your thoughts. I'm not entirely sure about the function signature as well. I'll add documentation once everything is finalized, thanks !

After revising all of our APIs, the method should be in_edge_indices and it should just be the logic of in_edges but returning only the EdgeIndices.

An optional improvement is adding out_edge_indices which is the same as out_edges but again for the other direction

@ThisuraGallage
Copy link

ThisuraGallage commented Jan 23, 2025

Hi, I added the requested changes in #1369 . I kept the tests for the undirected graph consistent with the other tests in the file. Let me know if those need the same improvement u suggested for tests in directed graph. ( I assumed the variables for edge indices were used to make the edge and directions clear in the test functions ). Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants