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.

Sign up for GitHub

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

Jump to bottom

BUG: fix Johnson's algorithm #17863

Merged
merged 1 commit into from
Feb 13, 2023
Merged

BUG: fix Johnson's algorithm #17863

merged 1 commit into from
Feb 13, 2023

Conversation

tsery-ns
Copy link
Contributor

Reference issue

Closes #14980

What does this implement/fix?

Remove unnecessary reset of dist_array during Bellman-Ford weight updates.
Add unit test for negative weighted shortest paths.

@tupui tupui added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse.csgraph labels Jan 26, 2023
@WarrenWeckesser WarrenWeckesser added the Cython Issues with the internal Cython code base label Jan 27, 2023
@j-bowhay j-bowhay added this to the 1.11.0 milestone Jan 29, 2023
@tsery-ns
Copy link
Contributor Author

Is there anything else I need to do towards merging this PR? @j-bowhay @tupui

@tupui
Copy link
Member

tupui commented Jan 31, 2023 •
edited
Loading

It's not my area so I would defer to other maintainers. There is a release flag so it will eventually get more attention.

@j-bowhay @WarrenWeckesser are you able to look into it?

@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Feb 10, 2023
tylerjereddy
tylerjereddy reviewed Feb 10, 2023
View reviewed changes
Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

This looks pretty solid to me. I confirmed that the original reproducer fails on latest main and passes on the branch here, and the new regression test looks pretty similar. I've added a backport label--this issue has been around since 1.7.x series it looks like.

The fact that no old tests were modified builds some confidence that there are no unintended breaks introduced by this, from the perspective of a non-expert anyway. There are a fair number of shortest_path tests in the suite already too.

We could ask @perimosocordiae to take a quick look--looks like they poked around here a while ago.

Even in the absence of an expert review though, this seems fairly solid.

scipy/sparse/csgraph/tests/test_shortest_path.py Outdated Show resolved Hide resolved
@tsery-ns
BUG: fix Johnson's algorithm.
75c0b9c 
Remove unnecessary reset of dist_array during Bellman-Ford
weight updates.
Add unit test for negative weighted shortest paths.
See scipy#14980
@tylerjereddy
Copy link
Contributor

This looks pretty solid and I think I'd like to backport it, so in it goes. More details on justification in comment above.

@tylerjereddy tylerjereddy merged commit b84f71e into scipy:main Feb 13, 2023
@tylerjereddy tylerjereddy modified the milestones: 1.11.0, 1.10.1 Feb 18, 2023
@tylerjereddy tylerjereddy mentioned this pull request Feb 18, 2023
Merged
2 tasks
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerjereddy tylerjereddy tylerjereddy left review comments

@j-bowhay j-bowhay j-bowhay left review comments

Assignees
No one assigned
Labels
Cython Issues with the internal Cython code base defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse.csgraph
Projects
None yet
Milestone
1.10.1
Development

Successfully merging this pull request may close these issues.

BUG: Johnson's algorithm fails without negative cycles
5 participants
@tsery-ns @tupui @tylerjereddy @j-bowhay @WarrenWeckesser