Early exit Dijsktra's with path reuse #466
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pedrocamargo
approved these changes
Nov 14, 2023
|
It looked correct to me, but this is an important change, so can you check as well, @jamiecook ? |
Add a private variable which tracks the status of the tree from the previous call to `compute_path`. Setting the `PathResults.early_exit` be used to modify the behaviour of the `compute_path` call in `update_trace`.
pedrocamargo
approved these changes
Nov 17, 2023
jamiecook
reviewed
Nov 22, 2023
jamiecook
reviewed
Nov 22, 2023
| @@ -60,6 +60,7 @@ def __init__(self) -> None: | |||
| self.origin = None | |||
| self.destination = None | |||
| self.graph: Graph = None | |||
| self.early_exit = False | |||
There was a problem hiding this comment.
Why do we have both _early_exit and early_exit?
There was a problem hiding this comment.
_early_exit is intended to privately mark the previous trees state for the path re-computation logic in update_trace. early_exit is intended to be a public configuration option, say if you want to change a PathResults object to use early exit on the next update trace where it previously hadn't or visa versa.
The early_exit public var could be dropped in favour of "tough luck call compute_path again".
I've also realised I've change this API slightly in the A* PR to be consistent
https://github.com/AequilibraE/aequilibrae/pull/467/files#diff-2639bd28c2998e1d08e05d2939351b7cece776c19fec0eaa9c020f98e2ec33e7L295-R349
pedrocamargo
added a commit
that referenced
this pull request
Nov 25, 2023
* fixes errors on qgis (#417) * runs examples (#422) Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> * Update LICENSE.TXT (#427) * fix matrix indices for memory-only matrices (#425) * fix matrix indices for memory-only matrices * run black --------- Co-authored-by: Pelle Koster <pelle.koster@nginfra.nl> * Pedro/cores skimming (#426) * Moves the setting of number of cores to the right place * Moves documentation to the right place --------- Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> * Updates assignment logging (#423) * assignment logging * Updates logging * Adds __config log * Adds test and docs * Update aequilibrae/paths/traffic_assignment.py * Fixes tests * fixes coverage testing * merges testing changes * style * style * style * style * narrows coverage test * Update test_traffic_assignment.py * Apply suggestions from code review * Fixes test_traffic_assignment --------- Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> Co-authored-by: Pedro Camargo <c@margo.co> * narrows coverage test (#429) Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> * moves openmatrix to a primary dependency (#431) * moves openmatrix to a primary dependency * QGIS moved to Numpy 1.24 before moving to Python 3.10 --------- Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> * Updates software version for release * Bumps up version * Bumps up documentation version --------- Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> * minor change to test CI * Fix multiple classes not being presented in the returned d (#438) * Prevent invalid attribute names on AequilibraE data fields This previously would have resulted in syntax error when accessing but its best we don't accept them anyway. * Fix multiple classes not being presented in the returned df Bug report: https://groups.google.com/g/aequilibrae/c/y_q9nLNs6-Y/m/yWXNpey9AAAJ * Style * Skip if network fails * Rounding * fixes test * Revert "Rounding" Vatican City really did move huh This reverts commit d9d0a5d. --------- Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> * Update README.md (#439) * Add implicit noexcept from Cython<3.0.0 (#440) With the release of Cython 3.0.0 there are a few changes of note to use. Particularly the performance impact of the removal of the implicit noexcept. Now all cdefs allow exceptions by default meaning every cdef must require the gil at the end of the function regardless of whether it was nogil or not. Adding the noexcept clause reverts to the old behaviour. There is a compiler directive to result this old behaviour but better be be explicit when the solution is one regex away: `^(cp?def(?:.|\n).*?)(nogil|):$` https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#exception-values-and-noexcept * Disable select link (#443) * Fix empty matrices not being saved (#444) Introduced in 7c2beb6 * Matrix exports and deprecation warning (#435) * Bumps up version * Bumps up documentation version * fixes matrix export * fixes deprecations * addresses SciPy versions * addresses SciPy versions * addresses SciPy versions * addresses SciPy versions * addresses SciPy versions * fixes test * fixes test --------- Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> * Select link correctness fix (#447) * Revert "Disable select link (#443)" This reverts commit 0cd3d48. * Add Kai Tang's test and data * Potential select link fix * Test formatting * Fix tests imports * Add select link test This test asserts that the results of the select link on the links 7, and 13 are the same as the results of the assignment. These links were chosen for this particular network to cover all paths used. * Prevent data races in select link results Memory for the multi-threaded runs are now allocated in MuliThreadedAoN along side the rest of the multi-threaded memory. * Installs package to run documentation pipeline * installing * Install pandoc --------- Co-authored-by: Pedro Camargo <c@margo.co> * bumps up the version (#451) Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> * new version * Fix documentation bugs on latest (#456) * Update requirements-docs.txt modifies pydata theme version * Update requirements-docs.txt * PathResults.compute_path fix stale variables (#457) Previously if a path that exists was computed, and then a path that does not exist was attempted, the old path variables were not cleared. * Replaces triggers with check statements (#455) * replaces triggers with check statements * Guarantees that links can be created without adding information on nodes --------- Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> * Adds zones and connectors (#461) * Better import from OSM and description of bicycle & bus data in network (#464) Parameters file is a bit hidden/tricky to use for qaequilibrae users, this should help them for a better default OSM import which keep data for bus and bicycle Have keep the same structure as OSM but this result in 3 fields for each one (3 for bicycle and 3 for bus). Maybe we can think of a way to achieve only one field for bicycle and one for bus ? Might not so easy because of the number of combinations available in OSM, example : https://wiki.openstreetmap.org/wiki/Key:cycleway Co-authored-by: Arthur Evrard <83211842+Art-Ev@users.noreply.github.com> * Early exit Dijsktra's with path reuse (#466) * Early exit Dijsktra's with path reuse * Remove early exit arg from update_trace, add private state Add a private variable which tracks the status of the tree from the previous call to `compute_path`. Setting the `PathResults.early_exit` be used to modify the behaviour of the `compute_path` call in `update_trace`. * Adds support for Python 3.12 (#453) * Adds support for Python 3.12 * Update build_mac.yml * Move from depreciated `license_file` as its now a build error * `assertEquals` is a deprecated alias for `assertEqual` --------- Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> Co-authored-by: Jake-Moss <jake.moss@uqconnect.edu.au> * Don't mark the path computation as `early_exit` when unreachable (#469) * Initial A* implementation (#467) * Initial A* implementation This implementation uses a haversine distance heuristic, in theory this a consistent heuristic [1] that would give us "an optimal path without processing any node more than once" when the graph cost is set to distance, however, in testing I've found that the distance calculations based and lon/lat done by myself and QGIS do not align with those in the project. Unfortunately this makes the heuristic not admissible [2] meaning A* will not always find the least-cost path, and may not do the minimum amount of work. From some basic instrumenting I found A* is consistently an order of magnitude better than the existing Dijkstra's implementation when it comes to heap operation counts. Time wise I haven't performed real benchmarks. Currently there is only one heuristic implemented for distance, if the graph cost field is set to `free_flow_time` the heuristic, while not useless is theory, it is practically as the magnitudes of the distances dominates any graph cost meaning A* will explore the whole network, just like Dijkstra's. [1] https://en.wikipedia.org/wiki/Consistent_heuristic [2] https://en.wikipedia.org/wiki/Admissible_heuristic * Indexing fixes found on Arkansas network * Updates graph types * A* improvements and corrections, equirectangular heuristic added * Update "without a model" example * fixup! A* improvements and corrections, equirectangular heuristic added * Set lon/lat index manually instead of passing to prepare_graph * fixup! Set lon/lat index manually instead of passing to prepare_graph * Split path computation and skimming example, use Coquimbo over Sioux * Add note about skimming with A* * General clean up * Extend existing test to include A* via subtests * Style * Add runtime heuristic switching * Document heuristic switching * Heuristic swithcing tests * Python 3.7 compatibility * Missed changes * Add note for A* non-distance metrics * Bumps up version for release --------- Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> --------- Co-authored-by: Renata Imai <53949163+r-akemii@users.noreply.github.com> Co-authored-by: pveigadecamargo <pveigadecamargo@anl.gov> Co-authored-by: PelleK <elfjes@users.noreply.github.com> Co-authored-by: Pelle Koster <pelle.koster@nginfra.nl> Co-authored-by: Jamie Cook <jimi.cook@gmail.com> Co-authored-by: Jake Moss <jake.moss@uqconnect.edu.au> Co-authored-by: Jamie Cook <jamie.cook@veitchlister.com.au> Co-authored-by: Arthur Evrard <83211842+Art-Ev@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a disabled by default option to exit once a destination is found in
path_findingtocompute_pathandupdate_trace.If a partial shortest path tree is computed,
update_tracewill check if the new destination has already been found within that tree, if it isn't it will recompute the necessary tree. It does this by marking the un-scanned nodes as unreachable upon exit, then if the new destination is unreachable and the tree is partial, the tree must be recomputed. Otherwise we are free to reuse the existing tree.I've added some tests that check the partial trees of the triangle example against what I worked out by hand.
I've also tested this as a default on option and all tests still pass.