Skip to content

Conversation

@uulm-janbaudisch
Copy link
Contributor

I am not sure if there was a reason for the constraint, but it seems to work fine for undirected graphs.

Fixes #588.

@uulm-janbaudisch uulm-janbaudisch changed the title Make Csr::from_sorted_edges generic over edge type feat: make Csr::from_sorted_edges generic over edge type May 2, 2025
@starovoid
Copy link
Collaborator

I think this was done some times ago due to the requirements of unambiguity of the edge order.
Thanks for the reminder.

@starovoid starovoid added this to the 0.8.3 milestone May 19, 2025
@uulm-janbaudisch
Copy link
Contributor Author

What I noticed when using CSRs for undirected graphs is that edges have to be present in both directions for from_sorted_edges to work correctly. This might be important for users. Maybe a note in the docs would be enough?

@starovoid starovoid force-pushed the csr-from-edges-undirected branch from 4b41b80 to a2b5892 Compare June 13, 2025 09:29
@starovoid starovoid added the C-feature-accepted Category: A feature request that has been accepted pending implementation label Jun 13, 2025
@starovoid starovoid force-pushed the csr-from-edges-undirected branch from a2b5892 to 5baf298 Compare June 22, 2025 17:13
@starovoid
Copy link
Collaborator

Maybe a note in the docs would be enough?

Let's do that.
And to ensure that everything works correctly and avoid future regressions, please write a test for constructing a Csr for an undirected graph.

@starovoid starovoid force-pushed the csr-from-edges-undirected branch from 5baf298 to 7d21f27 Compare June 29, 2025 11:39
@starovoid starovoid force-pushed the csr-from-edges-undirected branch from 7d21f27 to 2726f3c Compare July 13, 2025 18:57
@RaoulLuque
Copy link
Member

Since this PR seems stale, I was wondering if it would be alright if I would pick it up. That is, fork your branch and finish it such that your work gets merged : )

@uulm-janbaudisch uulm-janbaudisch force-pushed the csr-from-edges-undirected branch from bb207b2 to 26f106a Compare July 28, 2025 10:26
@uulm-janbaudisch
Copy link
Contributor Author

Sorry for the delay. I added a note to the documentation.
The added test is currently failing. The edge count seems to not be correctly calculated.
@RaoulLuque it would be great if you or someone else could take a look at it. I would be fine with you taking over the PR.

@uulm-janbaudisch uulm-janbaudisch force-pushed the csr-from-edges-undirected branch from 26f106a to b6f7190 Compare July 28, 2025 10:34
@RaoulLuque
Copy link
Member

RaoulLuque commented Jul 28, 2025

Sorry for the delay. I added a note to the documentation.
The added test is currently failing. The edge count seems to not be correctly calculated.
@RaoulLuque it would be great if you or someone else could take a look at it. I would be fine with you taking over the PR.

No worries at all :) Alright, I see 🤔 I'll look into it then and prob take over the PR 👍 I appreciate the quick response ☺️

@RaoulLuque
Copy link
Member

Okay, I will close this in favor of #861 then : )

@RaoulLuque RaoulLuque closed this Jul 29, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jul 31, 2025
… increase edge_count in Csr::from_sorted_edges (#861)

This PR continues the work done in #783. Thus, this resolves #588.

The trait bound on the `Csr` to be `Directed` for `from_sorted_edges`
was lifted. Thus, it was noticed that at no point in
`from_sorted_edges`, the `edge_count` is increased. This is only
relevant for `Undirected` graphs, as `Directed` graphs use their
`self.column.len()` as the `edge_count()` under the hood.

---------

Co-authored-by: Jan Baudisch <jan.baudisch@uni-ulm.de>
RaoulLuque added a commit to RaoulLuque/petgraph that referenced this pull request Sep 21, 2025
… increase edge_count in Csr::from_sorted_edges (petgraph#861)

This PR continues the work done in petgraph#783. Thus, this resolves petgraph#588.

The trait bound on the `Csr` to be `Directed` for `from_sorted_edges`
was lifted. Thus, it was noticed that at no point in
`from_sorted_edges`, the `edge_count` is increased. This is only
relevant for `Undirected` graphs, as `Directed` graphs use their
`self.column.len()` as the `edge_count()` under the hood.

---------

Co-authored-by: Jan Baudisch <jan.baudisch@uni-ulm.de>
RaoulLuque added a commit to RaoulLuque/petgraph that referenced this pull request Sep 21, 2025
… increase edge_count in Csr::from_sorted_edges (petgraph#861)

This PR continues the work done in petgraph#783. Thus, this resolves petgraph#588.

The trait bound on the `Csr` to be `Directed` for `from_sorted_edges`
was lifted. Thus, it was noticed that at no point in
`from_sorted_edges`, the `edge_count` is increased. This is only
relevant for `Undirected` graphs, as `Directed` graphs use their
`self.column.len()` as the `edge_count()` under the hood.

---------

Co-authored-by: Jan Baudisch <jan.baudisch@uni-ulm.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-feature-accepted Category: A feature request that has been accepted pending implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can we remove Directed trait constraint?

3 participants