Skip to content

Conversation

@CattleProdigy
Copy link
Contributor

Closes #870

The IntoEdges implementation for UnidirectedAdaptor does not produce the same set of edges references as an equivalently-constructed undirected graph. This prevents search and traversal algorithms from working if the traversal or search would require traversing an adapted edge whose underlying directed edge is "incoming" (which would prevent traversal in the directed case).

The underlying reason is that we currently, correctly concatenate incoming and outgoing edges from the underlying directed graph but they're concatenated with the same orientation which produces a set of redundant EdgeReferences-- making the adapted graph equivalent to the underlying directed graph as far as the IntoEdges implementation is concerned.

The IntoEdges implementation for undirected graphs produces two EdgeReferences for each graph edge-- one for each orientation. This is what allows traversal of edges in either direction.

The solution is to make the UndirectedAdaptor's IntoEdges behavior match that of an undirected graph by reversing the edges with Incoming orientation. That way there will be two EdgeReferences for each edge as there are in undirected graphs.

@CattleProdigy
Copy link
Contributor Author

All these lints are slated to be fixed here: #869

@starovoid starovoid force-pushed the undirectedadaptor-fix branch from 017304e to 654997f Compare August 26, 2025 17:29
Copy link
Collaborator

@starovoid starovoid left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the bugfix. I only have one suggestion for improving the test.

Closes petgraph#870

The `IntoEdges` implementation for `UnidirectedAdaptor` does not produce
the same set of edges references as an equivalently-constructed
undirected graph. This prevents search and traversal algorithms from
working if the traversal or search would require traversing an adapted
edge whose underlying directed edge is "incoming" (which would prevent
traversal in the directed case).

The underlying reason is that we currently, correctly concatenate
incoming and outgoing edges from the underlying directed graph but
they're concatenated with the same orientation which produces a set
of redundant `EdgeReferences`-- making the adapted graph equivalent
to the underlying directed graph as far as the `IntoEdges`
implementation is concerned.

The `IntoEdges` implementation for undirected graphs produces two
`EdgeReference`s for each graph edge-- one for each orientation. This is
what allows traversal of edges in either direction.

The solution is to make the `UndirectedAdaptor`'s `IntoEdges` behavior
match that of an undirected graph by reversing the edges with `Incoming`
orientation. That way there will be two `EdgeReferences` for each edge
as there are in undirected graphs.
@starovoid starovoid force-pushed the undirectedadaptor-fix branch from 654997f to b815451 Compare September 8, 2025 11:01
@starovoid starovoid added the C-bugfix Category: PR with bug fix label Sep 8, 2025
@starovoid starovoid added this to the 0.8.3 milestone Sep 8, 2025
@starovoid starovoid added this pull request to the merge queue Sep 10, 2025
Merged via the queue into petgraph:master with commit 01b17d9 Sep 10, 2025
11 checks passed
@github-actions github-actions bot mentioned this pull request Sep 10, 2025
RaoulLuque pushed a commit to RaoulLuque/petgraph that referenced this pull request Sep 21, 2025
…#871)

Closes petgraph#870

The `IntoEdges` implementation for `UnidirectedAdaptor` does not produce
the same set of edges references as an equivalently-constructed
undirected graph. This prevents search and traversal algorithms from
working if the traversal or search would require traversing an adapted
edge whose underlying directed edge is "incoming" (which would prevent
traversal in the directed case).

The underlying reason is that we currently, correctly concatenate
incoming and outgoing edges from the underlying directed graph but
they're concatenated with the same orientation which produces a set of
redundant `EdgeReferences`-- making the adapted graph equivalent to the
underlying directed graph as far as the `IntoEdges` implementation is
concerned.

The `IntoEdges` implementation for undirected graphs produces two
`EdgeReference`s for each graph edge-- one for each orientation. This is
what allows traversal of edges in either direction.

The solution is to make the `UndirectedAdaptor`'s `IntoEdges` behavior
match that of an undirected graph by reversing the edges with `Incoming`
orientation. That way there will be two `EdgeReferences` for each edge
as there are in undirected graphs.

<!--
  -- Thanks for contributing to `petgraph`! 
  --
-- We require PR titles to follow the Conventional Commits
specification,
-- https://www.conventionalcommits.org/en/v1.0.0/. This helps us
generate
  -- changelogs and follow semantic versioning.
  --
  -- Start the PR title with one of the following:
  --  * `feat:` for new features
  --  * `fix:` for bug fixes
  --  * `refactor:` for code refactors
  --  * `docs:` for documentation changes
  --  * `test:` for test changes
  --  * `perf:` for performance improvements
  --  * `revert:` for reverting changes
  --  * `ci:` for CI/CD changes
-- * `chore:` for changes that don't fit in any of the above categories
  -- The last two categories will not be included in the changelog.
  --
-- If your PR includes a breaking change, please add a `!` after the
type
-- and include a `BREAKING CHANGE:` line in the body of the PR
describing
  -- the necessary changes for users to update their code.
  --
  -->

---------

Co-authored-by: Paul Jakob Schroeder <paul.schroeder@tangramvision.com>
RaoulLuque pushed a commit to cactusdualcore/petgraph that referenced this pull request Sep 21, 2025
…#871)

Closes petgraph#870

The `IntoEdges` implementation for `UnidirectedAdaptor` does not produce
the same set of edges references as an equivalently-constructed
undirected graph. This prevents search and traversal algorithms from
working if the traversal or search would require traversing an adapted
edge whose underlying directed edge is "incoming" (which would prevent
traversal in the directed case).

The underlying reason is that we currently, correctly concatenate
incoming and outgoing edges from the underlying directed graph but
they're concatenated with the same orientation which produces a set of
redundant `EdgeReferences`-- making the adapted graph equivalent to the
underlying directed graph as far as the `IntoEdges` implementation is
concerned.

The `IntoEdges` implementation for undirected graphs produces two
`EdgeReference`s for each graph edge-- one for each orientation. This is
what allows traversal of edges in either direction.

The solution is to make the `UndirectedAdaptor`'s `IntoEdges` behavior
match that of an undirected graph by reversing the edges with `Incoming`
orientation. That way there will be two `EdgeReferences` for each edge
as there are in undirected graphs.

<!--
  -- Thanks for contributing to `petgraph`! 
  --
-- We require PR titles to follow the Conventional Commits
specification,
-- https://www.conventionalcommits.org/en/v1.0.0/. This helps us
generate
  -- changelogs and follow semantic versioning.
  --
  -- Start the PR title with one of the following:
  --  * `feat:` for new features
  --  * `fix:` for bug fixes
  --  * `refactor:` for code refactors
  --  * `docs:` for documentation changes
  --  * `test:` for test changes
  --  * `perf:` for performance improvements
  --  * `revert:` for reverting changes
  --  * `ci:` for CI/CD changes
-- * `chore:` for changes that don't fit in any of the above categories
  -- The last two categories will not be included in the changelog.
  --
-- If your PR includes a breaking change, please add a `!` after the
type
-- and include a `BREAKING CHANGE:` line in the body of the PR
describing
  -- the necessary changes for users to update their code.
  --
  -->

---------

Co-authored-by: Paul Jakob Schroeder <paul.schroeder@tangramvision.com>
RaoulLuque pushed a commit to RaoulLuque/petgraph that referenced this pull request Sep 21, 2025
…#871)

Closes petgraph#870

The `IntoEdges` implementation for `UnidirectedAdaptor` does not produce
the same set of edges references as an equivalently-constructed
undirected graph. This prevents search and traversal algorithms from
working if the traversal or search would require traversing an adapted
edge whose underlying directed edge is "incoming" (which would prevent
traversal in the directed case).

The underlying reason is that we currently, correctly concatenate
incoming and outgoing edges from the underlying directed graph but
they're concatenated with the same orientation which produces a set of
redundant `EdgeReferences`-- making the adapted graph equivalent to the
underlying directed graph as far as the `IntoEdges` implementation is
concerned.

The `IntoEdges` implementation for undirected graphs produces two
`EdgeReference`s for each graph edge-- one for each orientation. This is
what allows traversal of edges in either direction.

The solution is to make the `UndirectedAdaptor`'s `IntoEdges` behavior
match that of an undirected graph by reversing the edges with `Incoming`
orientation. That way there will be two `EdgeReferences` for each edge
as there are in undirected graphs.

<!--
  -- Thanks for contributing to `petgraph`! 
  --
-- We require PR titles to follow the Conventional Commits
specification,
-- https://www.conventionalcommits.org/en/v1.0.0/. This helps us
generate
  -- changelogs and follow semantic versioning.
  --
  -- Start the PR title with one of the following:
  --  * `feat:` for new features
  --  * `fix:` for bug fixes
  --  * `refactor:` for code refactors
  --  * `docs:` for documentation changes
  --  * `test:` for test changes
  --  * `perf:` for performance improvements
  --  * `revert:` for reverting changes
  --  * `ci:` for CI/CD changes
-- * `chore:` for changes that don't fit in any of the above categories
  -- The last two categories will not be included in the changelog.
  --
-- If your PR includes a breaking change, please add a `!` after the
type
-- and include a `BREAKING CHANGE:` line in the body of the PR
describing
  -- the necessary changes for users to update their code.
  --
  -->

---------

Co-authored-by: Paul Jakob Schroeder <paul.schroeder@tangramvision.com>
github-merge-queue bot pushed a commit that referenced this pull request Sep 30, 2025
## 🤖 New release

* `petgraph`: 0.8.2 -> 0.8.3 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

##
[0.8.3](https://github.com/petgraph/petgraph/compare/petgraph@v0.8.2...petgraph@v0.8.3)
- 2025-09-28

### Bug Fixes

- Infinite `subgraph_isomorphisms_iter` for empty isomorphisms
([#780](#780))
- Algos don't work on `UndirectedAdaptor`
([#870](#870))
([#871](#871))
- use a queue for SPFA
([#893](#893))
- `StableGraph::reverse` breaks free lists
([#890](#890))

### Documentation

- Fix examples link in README and unify typesetting of one word
([#823](#823))
- Add link to multigraph definition to isomorphism algos
([#824](#824))
- Fix auxiliary space (and time) complexity of bron-kerbosch
([#825](#825))
- Fix Typo in Operator Module Documentation
([#831](#831))
- Sync the crate feature flags in the README and docs
([#832](#832))
- Remove all \[Generic\] tags from algo docstrings
([#835](#835))
- Fix typos in comments
([#836](#836))
- Revamp CONTRIBUTING.md
([#833](#833))
- Update `GraphMap` link in README
([#857](#857))
- Add doc comment for `Dot::with_attr_getters`
([#850](#850))
- Specify iteration order for neighbors and edges and their variants
([#790](#790))
- Collection of Doc fixes
([#856](#856))

### New Features

- Add `into_nodes_edges_iters` to `StableGraph`
([#841](#841))
- Add methods to reserve & shrink `StableGraph` capacity
([#846](#846))
- Add Dinic's Maximum Flow Algorithm
([#739](#739))
- make Csr::from_sorted_edges generic over edge type and properly
increase edge_count in Csr::from_sorted_edges
([#861](#861))
- Add `map_owned` and `filter_map_owned` for `Graph` and `StableGraph`
([#863](#863))
- Add dijkstra::with_dynamic_goal
([#855](#855))
- Fix self-loop bug in all_simple_paths and enable multiple targets
([#865](#865))
- mark petgraph::dot::Dot::graph_fmt as public
([#866](#866))
- Add bidirectional Dijkstra algorithm
([#782](#782))

### Performance

- Make A* tie break on lower h-values
([#882](#882))

### Refactor

- add examples for scc algorithms and reorganize into dedicated module
([#830](#830))
- Remove unnecessary trait bounds from impls/methods
([#828](#828))
- replace uses of 'crate::util::zip' with 'core::iter::zip'
([#849](#849))
- Fix clippy (and other) lints
([#851](#851))
- Cleanup repo ([#854](#854))
- replace crate::util::enumerate with Iterator::enumerate
([#881](#881))

### Testing

- Add dependency list for 'quickcheck' feature
([#822](#822))
- Fix feature cfg capitalization in doctest
([#852](#852))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Egor Starovoitov <52821033+starovoid@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-bugfix Category: PR with bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Algorithms like A* don't work with UndirectedAdaptor

2 participants