Skip to content

Add longest_path_length to rustworkx_core::dag_algo#1605

Merged
IvanIsCoding merged 3 commits into
Qiskit:mainfrom
mtreinish:longest-path-length-dag-core
Jun 18, 2026
Merged

Add longest_path_length to rustworkx_core::dag_algo#1605
IvanIsCoding merged 3 commits into
Qiskit:mainfrom
mtreinish:longest-path-length-dag-core

Conversation

@mtreinish

Copy link
Copy Markdown
Member

This commit builds off of #1577 which improved the performance of the longest_path function to avoid allocating on each iteration of the loop. It was pointed out in that PR and #1563 that storing and returning a path for applications that only care about the length is unnecessary overhead. To provide an option for such cases a new function is added to offer a better alternative for these use cases.

This commit builds off of Qiskit#1577 which improved the performance of the
`longest_path` function to avoid allocating on each iteration of the
loop. It was pointed out in that PR and Qiskit#1563 that storing and returning
a path for applications that only care about the length is unnecessary
overhead. To provide an option for such cases a new function is added to
offer a better alternative for these use cases.
@mtreinish mtreinish added this to the 0.18.0 milestone Jun 15, 2026
@mtreinish mtreinish requested a review from jakelishman June 15, 2026 21:32
@mtreinish mtreinish mentioned this pull request Jun 16, 2026
3 tasks
Since path weights might be floating point numbers or other types which
don't implment Ord we need to handle the case where the comparison isn't
possible (e.g. for floats the presence of NaN). Previously we just were
treating this as equal which is not correct. This commit updates the
logic in the path length function to just return None in the presence of
non comparable elements along the longest path and adds documentation to
tell users about this and suggestions on how to handle this case in the
weight functions.
@mtreinish

Copy link
Copy Markdown
Member Author

I forgot to put in the commit message, but the reason for the seeming duplication here is that internally I changed the typing of the dist variable from a HashMap to a Vec which trades off faster access and iteration speed at the cost of potential over allocation in the presence of holes in the graph. However, I didn't want to make this change to the existing function because it requires changing the trait bounds to add NodeIndexable trait which would have been an API change for the existing function.

@mtreinish mtreinish requested a review from IvanIsCoding June 17, 2026 18:06

@IvanIsCoding IvanIsCoding left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. The tests are good, existing test pass. It should be an easy merge. It is bound to be faster than the code we had before.

@IvanIsCoding IvanIsCoding added this pull request to the merge queue Jun 18, 2026
Merged via the queue into Qiskit:main with commit 0b82f49 Jun 18, 2026
40 checks passed
@mtreinish mtreinish deleted the longest-path-length-dag-core branch June 18, 2026 01:35
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