-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
ENH: Add "limit" parameter to dijkstra calculation #2992
Conversation
Ping @jakevdp |
cool story, coveralls |
:) It's useful when you actually click the badge. |
It looks like it commented twice on the same commit. |
@@ -404,6 +409,10 @@ def dijkstra(csgraph, directed=True, indices=None, | |||
if np.any(indices < 0) or np.any(indices >= N): | |||
raise ValueError("indices out of range 0...N") | |||
|
|||
if not np.isscalar(limit): | |||
raise ValueError('limit must be numeric (float)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a TypeError
would be more appropriate.
On second thought, can't limit
be declared a double
variable in the function prototype? That way, Cython generates the proper exception and the limit = float(limit)
is no longer needed.
Thanks @larsmans, fixed now. |
The maximum distance to calculate. Using a smaller limit will | ||
decrease computation time by aborting calculations between pairs | ||
that are separated by a distance > limit. | ||
.. versionadded:: 0.14.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc string should specify that any pair with distance > limit will be set to distance = infinity.
Also, in the code we should enforce limit >= 0, otherwise this claim won't be true!
Really nice work here. A simple change, and it adds some nice flexibility to the algorithm. Examining the code, it looks to be algorithmically correct, and it won't slow down the default case. One potential addition that comes to mind (which would be much less straightforward to implement): an option to allow building a sparse rather than dense output matrix. Because you don't know from the beginning how many nodes would be included, it would require dynamically expanding the array during the algorithm. I don't think that's worth tackling right now, but I thought I'd mention it! |
@jakevdp thanks for the comments, they have been (hopefully) addressed by the latest commit. Let me know if you find any other issues. I agree that a sparse representation could be useful. It's actually what we end up converting our representation to eventually anyway. Probably good for a Sunday hack PR at some point. |
@jakevdp anything else I need to do to move this toward merge? Or are we just waiting for another person or two to have a look? |
@@ -49,6 +58,21 @@ | |||
|
|||
|
|||
@dec.skipif(np.version.short_version < '1.6', "Can't test arrays with infs.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These version-conditional tests are going to stop running when numpy version is up to 1.10.x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Should this become a new PR, or do you want me to tackle this? Shouldn't be hard with LooseVersion
from distutils
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that existed before this PR. It should be fixed, but probably under a separate issue.
Edit: opened in #2999
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They will, we have to fix that in multiple places at once for the next release. I'll open an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad we're on the same page:)
@rgommers any other thoughts beyond the version testing? Sorry to keep pinging, I'm just waiting for this to be merged before a PR in a different package can be merged. |
Looks good to me. Did some testing with invalid inputs, all gives the expected errors. Merging. Thanks Eric, all |
ENH: Add "limit" parameter to dijkstra calculation
This is pretty straightforward. Tests pass, and I have also tried it with a real example with > 100,000 nodes. Results are identical (to numerical precision) with C code someone else wrote. Using a
limit
in my use-case speeds up computation by a factor of 20.