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.

Sign up for GitHub

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

Jump to bottom

distance_influence should not get a default value if no value is specified #2716

Merged
merged 5 commits into from Jan 10, 2023
Merged

distance_influence should not get a default value if no value is specified #2716

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2091d33
distance_influence should only be sent to server when explicitly spec…
karussell Jan 6, 2023
1a32582
fix changelog
karussell Jan 6, 2023
3a443b5
changelog
karussell Jan 6, 2023
8ae8077
CustomModel.getDistanceInfluence: allow null values
karussell Jan 9, 2023
945f484
remove comment
karussell Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
@@ -1,5 +1,6 @@
### 7.0 [not yet released]

- there is no longer a default value for the distanceInfluence Parameter in custom models sent via client-hc. Previously it was 70. Not setting it explicitly now means the server-side value will be used. Call hasDistanceInfluence before getDistanceInfluence, see #2716
- there is a new, required 'import.osm.ignored_highways' configuration option that must be used to not increase the graph size and decrease performance for motorized-only routing compared to previous versions, #2702
- new osm_way_id encoded value, #2701
- the parameters vehicle, weighting, edge_based and turn_costs are no longer supported, use the profile parameter instead
Expand Down
2 changes: 1 addition & 1 deletion client-hc/src/test/java/com/graphhopper/api/GraphHopperWebTest.java
View file
Open in desktop
Expand Up @@ -95,7 +95,7 @@ public void customModel() throws JsonProcessingException {
.addToSpeed(Statement.If("road_class == MOTORWAY", Statement.Op.LIMIT, "80"))
.addToPriority(Statement.If("surface == DIRT", Statement.Op.MULTIPLY, "0.7"))
.addToPriority(Statement.If("surface == SAND", Statement.Op.MULTIPLY, "0.6"))
.setDistanceInfluence(69)
.setDistanceInfluence(69d)
.setHeadingPenalty(22)
.setAreas(areas);
GHRequest req = new GHRequest(new GHPoint(42.509225, 1.534728), new GHPoint(42.512602, 1.551558))
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/com/graphhopper/routing/weighting/custom/CustomModelParser.java
View file
Open in desktop
Expand Up @@ -115,7 +115,7 @@ static CustomWeighting.Parameters createWeightingParameters(CustomModel customMo
CustomWeightingHelper prio = (CustomWeightingHelper) clazz.getDeclaredConstructor().newInstance();
prio.init(lookup, avgSpeedEnc, priorityEnc, customModel.getAreas());
return new CustomWeighting.Parameters(prio::getSpeed, prio::getPriority, prio.getMaxSpeed(), prio.getMaxPriority(),
customModel.getDistanceInfluence(), customModel.getHeadingPenalty());
customModel.hasDistanceInfluence() ? customModel.getDistanceInfluence() : 0, customModel.getHeadingPenalty());
Copy link
Member

Choose a reason for hiding this comment

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

Maybe extract a constant as this 0 is the server-side default value and it appears also in FindMinMax.

} catch (ReflectiveOperationException ex) {
throw new IllegalArgumentException("Cannot compile expression " + ex.getMessage(), ex);
}
Expand Down
7 changes: 4 additions & 3 deletions core/src/main/java/com/graphhopper/routing/weighting/custom/FindMinMax.java
View file
Open in desktop
Expand Up @@ -23,10 +23,11 @@ public class FindMinMax {
public static void checkLMConstraints(CustomModel baseModel, CustomModel queryModel, EncodedValueLookup lookup) {
if (queryModel.isInternal())
throw new IllegalArgumentException("CustomModel of query cannot be internal");
if (queryModel.hasDistanceInfluence() && queryModel.getDistanceInfluence() < baseModel.getDistanceInfluence())
double qmDI = queryModel.hasDistanceInfluence() ? queryModel.getDistanceInfluence() : 0;
double bmDI = baseModel.hasDistanceInfluence() ? baseModel.getDistanceInfluence() : 0;
Comment on lines +26 to +27
Copy link
Member

Choose a reason for hiding this comment

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

Here the 0 is also the server-side default? Shouldn't there really only be one place where it appears: The code where the yaml definition of the model is translated to a Java object?

Copy link
Member Author

@karussell karussell Jan 10, 2023 •
edited

Choose a reason for hiding this comment

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

Here the 0 is also the server-side default?

Not sure. Isn't it more an artifact of the "Double" type? It would be implicit for "double".

The code where the yaml definition of the model is translated to a Java object?

Even if your request is without (de)serialization you can still have null values.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I thought after reading the config file the distance influence field of the server-side model should either be 0 (if nothing was specified) or whatever was specified in the server config, but never null. And when we merge a request custom model with the one on the server-side we either use the one from the server (in case the requesting distance influence is null ) or use the value from the request (in case it was not null).

So basically I think that baseModel.getDistanceInfluence() == null would be a programming error and can be treated as such just let the NullPointerException be thrown.

And for qmDI we don't need a default value, because when nothing is given we will use the server-side value. Written as code:

        if (queryModel.getDistanceInfluence() != null && queryModel.getDistanceInfluence() < baseModel.getDistanceInfluence())
            throw new IllegalArgumentException("CustomModel in query can only use " +
                    "distance_influence bigger or equal to " + baseModel.getDistanceInfluence() + ", but was: " + queryModel.getDistanceInfluence());

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see this is not the way we do it currently. We do not apply the default server-side value during the deserialization of config.yml, but only much later in createWeightingParameters. This way it is also applied when we use the Java API. And because we call FindMinMax even before that we need to pay attention that there is no value yet :( Maybe it would be cleaner to use two separate classes for the server-side/profile CustomModel and the one used for the request.

if (qmDI < bmDI)
throw new IllegalArgumentException("CustomModel in query can only use " +
"distance_influence bigger or equal to " + baseModel.getDistanceInfluence() +
", but was: " + queryModel.getDistanceInfluence());
"distance_influence bigger or equal to " + bmDI + ", but was: " + qmDI);

checkMultiplyValue(queryModel.getPriority(), lookup);
checkMultiplyValue(queryModel.getSpeed(), lookup);
Expand Down
12 changes: 6 additions & 6 deletions core/src/test/java/com/graphhopper/GraphHopperTest.java
View file
Open in desktop
Expand Up @@ -271,7 +271,7 @@ private void testImportCloseAndLoad(boolean ch, boolean lm, boolean sort, boolea
new Coordinate(7.4198, 43.7355),
new Coordinate(7.4207, 43.7344),
new Coordinate(7.4174, 43.7345)}));
CustomModel customModel = new CustomModel().setDistanceInfluence(0);
CustomModel customModel = new CustomModel().setDistanceInfluence(0d);
customModel.getPriority().add(Statement.If("in_area51", Statement.Op.MULTIPLY, "0.1"));
customModel.getAreas().put("area51", area51Feature);
profile = new CustomProfile(profileName).setCustomModel(customModel).setVehicle(vehicle);
Expand Down Expand Up @@ -692,8 +692,8 @@ public void testCustomModel() {
);
assertDistance(hopper, customCar, customModelWithUnclassifiedRule, 19289);
// now we use distance influence to avoid the detour
assertDistance(hopper, customCar, new CustomModel(customModelWithUnclassifiedRule).setDistanceInfluence(200), 8725);
assertDistance(hopper, customCar, new CustomModel(customModelWithUnclassifiedRule).setDistanceInfluence(100), 14475);
assertDistance(hopper, customCar, new CustomModel(customModelWithUnclassifiedRule).setDistanceInfluence(200d), 8725);
assertDistance(hopper, customCar, new CustomModel(customModelWithUnclassifiedRule).setDistanceInfluence(100d), 14475);
}

private void assertDistance(GraphHopper hopper, String profile, CustomModel customModel, double expectedDistance) {
Expand Down Expand Up @@ -1631,8 +1631,8 @@ public void testLMConstraintsForCustomProfiles() {
setGraphHopperLocation(GH_LOCATION).
setOSMFile(MONACO).
setProfiles(
new CustomProfile("p1").setCustomModel(new CustomModel().setDistanceInfluence(100)).setVehicle("car"),
new CustomProfile("p2").setCustomModel(new CustomModel().setDistanceInfluence(100)).setVehicle("car")).
new CustomProfile("p1").setCustomModel(new CustomModel().setDistanceInfluence(100d)).setVehicle("car"),
new CustomProfile("p2").setCustomModel(new CustomModel().setDistanceInfluence(100d)).setVehicle("car")).
setStoreOnFlush(true);

hopper.getLMPreparationHandler().setLMProfiles(new LMProfile("p1"));
Expand All @@ -1644,7 +1644,7 @@ public void testLMConstraintsForCustomProfiles() {
assertEquals(3587, response.getBest().getDistance(), 1);

// use smaller distance influence to force violating the LM constraint
final CustomModel customModel = new CustomModel().setDistanceInfluence(0);
final CustomModel customModel = new CustomModel().setDistanceInfluence(0d);
response = hopper.route(new GHRequest(43.727687, 7.418737, 43.74958, 7.436566).
setCustomModel(customModel).
setProfile("p1").putHint("lm.disable", false));
Expand Down
8 changes: 4 additions & 4 deletions core/src/test/java/com/graphhopper/reader/osm/GraphHopperOSMTest.java
View file
Open in desktop
Expand Up @@ -816,7 +816,7 @@ public void testProfilesMustNotBeChanged() {
{
GraphHopper hopper = createHopperWithProfiles(Arrays.asList(
new Profile("car").setVehicle("car").setWeighting("fastest"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3)).setVehicle("car")
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3d)).setVehicle("car")
));
hopper.importOrLoad();
hopper.close();
Expand All @@ -825,7 +825,7 @@ public void testProfilesMustNotBeChanged() {
// load without problem
GraphHopper hopper = createHopperWithProfiles(Arrays.asList(
new Profile("car").setVehicle("car").setWeighting("fastest"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3)).setVehicle("car")
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3d)).setVehicle("car")
));
hopper.importOrLoad();
hopper.close();
Expand All @@ -834,7 +834,7 @@ public void testProfilesMustNotBeChanged() {
// problem: the profile changed (slightly). we do not allow this because we would potentially need to re-calculate the subnetworks
GraphHopper hopper = createHopperWithProfiles(Arrays.asList(
new Profile("car").setVehicle("car").setWeighting("fastest"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(80)).setVehicle("car")
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(80d)).setVehicle("car")
));
IllegalStateException e = assertThrows(IllegalStateException.class, hopper::importOrLoad);
assertTrue(e.getMessage().contains("Profiles do not match"), e.getMessage());
Expand All @@ -844,7 +844,7 @@ public void testProfilesMustNotBeChanged() {
// problem: we add another profile, which is not allowed, because there would be no subnetwork ev for it
GraphHopper hopper = createHopperWithProfiles(Arrays.asList(
new Profile("car").setVehicle("car").setWeighting("fastest"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3)).setVehicle("car"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3d)).setVehicle("car"),
new Profile("car2").setVehicle("car").setWeighting("fastest")
));
IllegalStateException e = assertThrows(IllegalStateException.class, hopper::importOrLoad);
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/com/graphhopper/routing/RoutingAlgorithmWithOSMTest.java
View file
Open in desktop
Expand Up @@ -130,7 +130,7 @@ public void testMonacoMotorcycleCurvature() {
queries.add(new Query(43.733802, 7.413433, 43.739662, 7.424355, 2423, 141));
queries.add(new Query(43.730949, 7.412338, 43.739643, 7.424542, 2253, 120));
queries.add(new Query(43.727592, 7.419333, 43.727712, 7.419333, 0, 1));
CustomModel model = new CustomModel().addToPriority(Statement.If("true", MULTIPLY, "curvature"));
CustomModel model = new CustomModel().setDistanceInfluence(70d).addToPriority(Statement.If("true", MULTIPLY, "curvature"));

GraphHopper hopper = createHopper(MONACO, new CustomProfile("motorcycle").setCustomModel(model).
setVehicle("motorcycle").setWeighting("custom"));
Expand Down