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

Change RelaxHierarchyLimits with pre-defined algorithm-based factors #3253

Merged

Conversation

mloskot
Copy link
Collaborator

@mloskot mloskot commented Aug 8, 2021

Issue

Single place of definition of the hard-coded factors which apparently
are re-used in at least two locations.

This also fixes a inconsistency or bug/regression that was introduced around two commits

which pass a boolean to RelaxHierarchyLimits, instead of float-point factor
which was later cultivated unnoticed in PR #2719

Maintaining the factors in single place should help prevent such situations.

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

@mloskot mloskot force-pushed the ml/fix-relaxhierarchylimits-factor-regression branch from b1c8e4f to 45de747 Compare August 8, 2021 15:20
Single place of definition of the hard-coded factors which apparently
are re-used in at least two locations.

This also fixes a inconsistency or bug/regression that was introduced around  two commits
- valhalla@1b150f7
- valhalla@9b64608
which pass a boolean to RelaxHierarchyLimits,  instead of float-point factor.
which was later cultivated unnoticed in PR valhalla#2719

Maintaining the factors in single place should help prevent such situations.
src/sif/dynamiccost.cc Outdated Show resolved Hide resolved
src/sif/dynamiccost.cc Outdated Show resolved Hide resolved
src/sif/dynamiccost.cc Outdated Show resolved Hide resolved
@mloskot mloskot changed the title Add RelaxHierarchyLimits with pre-defined algorithm-based factors Change RelaxHierarchyLimits with pre-defined algorithm-based factors Aug 9, 2021
test/astar.cc Outdated Show resolved Hide resolved
@mloskot mloskot force-pushed the ml/fix-relaxhierarchylimits-factor-regression branch from 187cf48 to f136f22 Compare August 10, 2021 12:18
@mloskot
Copy link
Collaborator Author

mloskot commented Aug 10, 2021

@kevinkreiser Shall I be concerned about this failure of lint-build-debug?

Suppressed 236 warnings (236 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
diff --git a/src/valhalla_run_route.cc b/src/valhalla_run_route.cc
index 9f7503f97..334b0e286 100644
--- a/src/valhalla_run_route.cc
+++ b/src/valhalla_run_route.cc
@@ -131,7 +131,6 @@ const valhalla::TripLeg* PathTest(GraphReader& reader,
                                   PathStatistics& data,
                                   bool multi_run,
                                   uint32_t iterations,
-                                  bool using_astar,
                                   bool using_bd,
                                   bool match_test,
                                   const std::string& routetype,
@@ -716,7 +715,7 @@ int main(int argc, char* argv[]) {
     const valhalla::TripLeg* trip_path = nullptr;
     try {
       trip_path = PathTest(reader, origin, dest, pathalgorithm, mode_costing, mode, data, multi_run,
-                           iterations, using_astar, using_bd, match_test, routetype, request);
+                           iterations, using_bd, match_test, routetype, request);
     } catch (std::runtime_error& rte) { LOG_ERROR("trip_path not found"); }
 
     // If successful get directions
Tidy introduced changes

BTW, I did run clang-format before the current last commit.

@kevinkreiser
Copy link
Member

kevinkreiser commented Aug 10, 2021

@mloskot you should indeed make the changes that it proposes. its basically saying, you dont make use of this parameter at all so why not remove it completely from the function

i should mention that the way ci is setup, even if you didnt modify that part of the code clang tidy will trigger on it. we introduced clang tidy maybe a few years ago and the idea was that we would slowly clean up the code as we touch different files. which means if you modify a file that hasnt be "tidied" then you get the fun of making the tidy changes too even if its code you didnt touch 😄

@mloskot
Copy link
Collaborator Author

mloskot commented Aug 10, 2021

@kevinkreiser

even if you didnt modify that part of the code clang tidy will trigger on it.

I see. It makes sense now. Fixed in d854cce

@mloskot mloskot merged commit 79c4046 into valhalla:master Aug 10, 2021
@mloskot mloskot deleted the ml/fix-relaxhierarchylimits-factor-regression branch August 10, 2021 15:31
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

2 participants