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

Feature #377 alternative routes #403

Closed
wants to merge 3 commits into from

Conversation

takb
Copy link
Contributor

@takb takb commented Jan 28, 2019

Pull Request Checklist

  • 1. I have merged the latest version of the development branch into my feature branch and all conflicts have been resolved.
  • 2. I have added information about the change/addition to functionality to the CHANGELOG.md file under the [Unreleased] heading.
  • 3. I have documented my code using JDocs tags.
  • 4. I have removed unnecessary commented out code, imports and System.out.println statements.
  • 5. I have written JUnit tests for any new methods/classes and ensured that they pass.
  • 6. I have created API tests for any new functionality exposed to the API.
  • 7. If changes/additions are made to the app.config file, I have added these to the app.config.sample file along with a short description of what it is for, and documented this in the Pull Request (below).
  • 8. I have built graphs with my code of the Heidelberg.osm.gz file and run the api-tests with all test passing
  • 9. I have referenced the Issue Number in the Pull Request (if the changes were from an issue).
  • 10. For new features or changes involving building of graphs, I have tested on a larger dataset (at least Germany) and the graphs build without problems (i.e. no out-of-memory errors).
  • 11. For new features or changes involving the graphbuilding process (i.e. changing encoders, updating the importer etc.), I have generated longer distance routes for the affected profiles with different options (avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end points generated from the current live ORS. If there are differences then the reasoning for these MUST be documented in the pull request.
  • 12. I have written in the Pull Request information about the changes made including their intended usage and why the change was needed.

Fixes #377 .

Information about the changes

  • Key functionality added:

  • routing API option "alternative_routes": n (int) added, uses GH alternative_route algorithm to return up to n routes for the query. Currently disables CH, and more than 2 waypoints are not supported. Parameters for the algorithm can be set with options "alternative_routes_weight_factor": (double) and "alternative_routes_share_factor": (double).

  • Reason for change:
    This gets the algorithm to work basically, but a) the algorithm produces some funny results and b) CH support is underway (Faster alternative routes for longer routes graphhopper/graphhopper#1524), so we'll need to continue work on this feature in a while. The changes to RouteResultBuilder are also crying out for serious refactoring... So this is not done or anything, just stashing away interim results.

@takb takb requested a review from rabidllama January 29, 2019 10:29
Copy link
Contributor

@rabidllama rabidllama left a comment

Choose a reason for hiding this comment

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

Mostly looks good, though I think we should do a small amount of refactoring in the RouteResultBuilder to reduce the duplication of the code that builds some of the route information.

There is a line of code in RoutingProfileManager (ln 463) that defines whether a route uses dynamic weights, which in turn is used to limit the length of routes requested. As the alternative routing algorithm does not used the optimised routing algorithms, we should add to that line a check for requesting alternative routes and if they are requested, then the distance check should also be carried out.

RouteResult[] resultSet = new RouteResult[route.getAll().size()];

for (PathWrapper path : route.getAll()) {
RouteResult result = new RouteResult(request.getExtraInfo());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest that we refactor some of the code to reduce duplication of methods. For example, a lot of the code here is duplicated earlier, so it would be good if we could take this code and put it into a method that can be used for all places where this functionality is required. There are some differences relating to some aspects (e.g. loops), so it might need a bit of thought to get it working properly but it would certainly be a step in the right direction for cleaning up this class.

@@ -65,6 +65,11 @@
private int[] _avoidCountries = null;
private BordersExtractor.Avoid _avoidBorders = BordersExtractor.Avoid.NONE;

// TAKB: parameters weight factor and share factor seem to be ignored by the algorithm, further testing required.
private int _alternativeRoutes = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better to name this _alternativeRouteCount to make it clearer what it is for (same for the getter and setter)

@takb takb force-pushed the feature-#377-alternative-routes branch from 6e9a817 to 909d33c Compare February 5, 2019 08:10
…for GHResponse with multiple paths, minor refactoring in RouteResultBuilder and RoutingRequestProcessor
@takb takb force-pushed the feature-#377-alternative-routes branch from 6088d01 to d53734a Compare February 5, 2019 08:33
@takb takb force-pushed the feature-#377-alternative-routes branch 2 times, most recently from 21650de to 42ee583 Compare February 18, 2019 09:43
@takb takb force-pushed the feature-#377-alternative-routes branch from 42ee583 to 74a086f Compare February 18, 2019 11:44
@takb takb closed this Mar 25, 2019
@takb takb deleted the feature-#377-alternative-routes branch November 28, 2019 13:59
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.

2 participants