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

No longer enable CH by default in GraphHopper class #1914

Merged
merged 7 commits into from
Feb 17, 2020
Merged

Conversation

easbar
Copy link
Member

@easbar easbar commented Feb 16, 2020

So far when creating a new GraphHopper instance the CHPreparationHandler was set to be enabled and "fastest" was used as default weighting, resulting in a "fastest" CH preparation for every vehicle. In this PR I have removed this default so there no longer are any CH preparations by default. I also removed the LMPreparationHandler#setEnabled methods, because there are only two cases: Either some CH/LM profiles are configured (then CH/LM is enabled) or not (then CH/LM is disabled), which means setEnabled(false) is equivalent to setting up no CH/LMProfiles and setEnabled(true) requires setting up the profiles and is thus unnecessary.

The usage goes like this now:

graphhopper:
   # nothing changed here: adding some weightings means they will be prepared using CH
   prepare.ch.weightings=fastest
   # using an empty string or setting to 'no' means the list of weightings is empty, just as before this PR
   prepare.lm.weightings=no
// create a new GraphHopper object. note that compared to before this PR there is no CH profile
GraphHopper hopper = new GraphHopper();
// these methods are all gone
// hopper.setCHEnabled(true);
// hopper.getCHPreparationHandler().setEnabled(true);
// hopper.getLMPreparationHandler().setEnabled(false);

// to enable CH/LM we need to add the profiles
// this resembles the previous behavior (CH will be 'enabled' by adding the profile)
hopper.getCHPreparationHandler().setCHProfileStrings("fastest");

* for any usage of GraphHopper there should either be a setCHEnabled(false) call or some CH profile strings should be set

* -> now we can remove it again :)
* remove enabled flag in CH/LMPreparationHandlers
@easbar easbar added this to the 1.0 milestone Feb 16, 2020
Comment on lines 51 to 52
// todonow: do we want the (previously by default) CH profiles here? (same question for all GraphHopperGtfs basically
ghConfig.put("prepare.ch.weightings", "fastest");
Copy link
Member Author

Choose a reason for hiding this comment

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

@michaz ? I wasn't sure if CH was used for this test only by accident (because of the default) or if its actually intended.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like an accident, pt does not use CH -> removed this

@@ -17,10 +17,10 @@ wget http://download.geofabrik.de/europe/germany/brandenburg-latest.osm.pbf
# The following process will take roughly 5 minutes on a modern laptop when it is executed for the first time.
# It imports the previously downloaded OSM data of the Brandenburg area as well as the GTFS.
java -Xmx8g -Xms8g \
# todonow: this is a) outdated, and b) why no CH (when in tests its enabled?)
Copy link
Member Author

Choose a reason for hiding this comment

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

@michaz ? We probably want to update the command line parameters to -Ddw.graphhopper.xyz (since -Dgraphhopper.xyz is no longer working after #1879), and I was wondering why we are setting ch.weightings=no here (while for some gtfs tests we were using CH) (see other comment)

Copy link
Member Author

@easbar easbar Feb 17, 2020

Choose a reason for hiding this comment

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

Updated the readme and removed ch.weightings=no as this is now the default

@easbar easbar merged commit a05de4b into master Feb 17, 2020
@easbar easbar deleted the no_ch_by_default branch February 17, 2020 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant