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.

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

Cost matrix early abort #3611

Merged
merged 9 commits into from
May 19, 2022
Merged

Conversation

monteirodom
Copy link
Contributor

Shorten down the request delay, when some sources/targets searches are early aborted.

  • Decrease remaining_sources_ for all targets when a source search is aborted due to overpassing the threshold
  • Decrease remaining_targets_ for all sources when a target search is aborted due to overpassing the threshold

Issue

#3610

@kevinkreiser
Copy link
Member

looks like this change is failing a test:

FAIL] matrix
[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 2 tests from Matrix
[ RUN      ] Matrix.test_matrix
/root/project/test/matrix.cc:238: Failure
The difference between results[i].dist and matrix_answers[i].dist is 99996361, which exceeds kThreshold, where
results[i].dist evaluates to 100000000,
matrix_answers[i].dist evaluates to 3639, and
kThreshold evaluates to 1.
result 7's distance is not close enough to expected value for CostMatrix
/root/project/test/matrix.cc:242: Failure
The difference between results[i].time and matrix_answers[i].time is 99996056, which exceeds kThreshold, where
results[i].time evaluates to 100000000,
matrix_answers[i].time evaluates to 3944, and
kThreshold evaluates to 1.
result 7's time is not close enough to expected value for CostMatrix

your description of what you wanted to do made sense to me but it looks like the tests show that now certain pairs of locations that used to get results dont get results anymore (since they come back with that absurdly high number). if you need help learning how to run the tests let me know so you can debug it and get it workign the way you want

@monteirodom
Copy link
Contributor Author

monteirodom commented May 11, 2022

Yeah, I've seen the tests failing ... no idea why the changes make some good results to be broken.
Was trying to have the tests run locally but got some errors at compilation (on macOS).
Tried on docker, but failed also at 74% of compilation. Here's the last lines of the compilation

#14 2337.1 [ 72%] Linking CXX executable gurka_route_summary
#14 2350.2 [ 72%] Built target gurka_route_summary
#14 2350.3 [ 72%] Generating scripts.log
#14 2350.4 + /usr/bin/python3.8 -m unittest discover -s /src/valhalla/test/scripts -v
#14 2359.9 make[3]: *** [test/scripts/CMakeFiles/run-scripts.dir/build.make:75: test/scripts/scripts.log] Error 1
#14 2360.1 make[2]: *** [CMakeFiles/Makefile2:16848: test/scripts/CMakeFiles/run-scripts.dir/all] Error 2
#14 2360.1 make[2]: *** Waiting for unfinished jobs....
#14 2386.0 [ 72%] Linking CXX executable gurka_route
#14 2392.9 [ 72%] Built target gurka_route
#14 2398.4 [ 72%] Linking CXX executable gurka_route_with_narrative_languages
#14 2400.5 [ 72%] Built target gurka_route_with_narrative_languages
#14 2400.8 [ 73%] Linking CXX executable gurka_search_side_of_street
#14 2414.6 [ 73%] Built target gurka_search_side_of_street
#14 2426.2 [ 73%] Linking CXX executable gurka_simple_restrictions
#14 2427.9 [ 73%] Linking CXX executable gurka_service_turn
#14 2428.0 [ 73%] Built target gurka_simple_restrictions
#14 2429.3 [ 73%] Built target gurka_service_turn
#14 2430.8 [ 73%] Linking CXX executable gurka_phonemes
#14 2432.0 [ 73%] Built target gurka_phonemes
#14 2432.0 [ 74%] Linking CXX executable gurka_stop_signs
#14 2433.3 [ 74%] Built target gurka_stop_signs
#14 2433.4 make[1]: *** [CMakeFiles/Makefile2:2404: test/CMakeFiles/check.dir/rule] Error 2
#14 2433.6 make: *** [Makefile:735: check] Error 2

Took off the gurka and scripts testing parts, then the tests do compile now. On the way to check the errors ...

@kevinkreiser kevinkreiser self-requested a review May 17, 2022 15:33
kevinkreiser
kevinkreiser previously approved these changes May 17, 2022
Copy link
Member

@kevinkreiser kevinkreiser 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 good to me, @monteirodom can you put a changelog entry linking this pr to the bottom of the enhancements list. thanks again!

@dnesbitt61
Copy link
Member

did a quick comparison using valhalla_run_matrix and it looks good.

kevinkreiser
kevinkreiser previously approved these changes May 17, 2022
@kevinkreiser
Copy link
Member

looks like theres a conflict with master, probably in the changelog and i dont have permissions to fix it on your branch. but once you resolve the conflict we can get your PR merged! thanks again for hanging in there!

CHANGELOG.md Outdated
@@ -83,7 +83,8 @@
* CHANGED: modernized spatialite syntax [#3580](https://github.com/valhalla/valhalla/pull/3580)
* ADDED: Options to generate partial results for time distance matrix when there is one source (one to many) or one target (many to one). [#3181](https://github.com/valhalla/valhalla/pull/3181)
* ADDED: Enhance valhalla_build_elevation with LZ4 recompression support [#3607](https://github.com/valhalla/valhalla/pull/3607)
* CHANGED: removed UK admin and upgraded its constituents to countries [#3619](https://github.com/valhalla/valhalla/pull/3619) * CHANGED: expansion service: only track requested max time/distance [#3532](https://github.com/valhalla/valhalla/pull/3509)
* ADDED: Shorten down the request delay, when some sources/targets searches are early aborted [#3611](https://github.com/valhalla/valhalla/pull/3611)
Copy link
Member

Choose a reason for hiding this comment

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

can you move this to the end of the list please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

@kevinkreiser kevinkreiser merged commit 022899c into valhalla:master May 19, 2022
@monteirodom monteirodom deleted the cost_matrix_early_abort branch May 23, 2022 07:47
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.

None yet

3 participants