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

Take rental vehicle battery level into account #5960

Draft
wants to merge 7 commits into
base: dev-2.x
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package org.opentripplanner.astar.strategy;

import java.util.function.Predicate;
import org.opentripplanner.astar.spi.AStarEdge;
import org.opentripplanner.astar.spi.AStarState;
import org.opentripplanner.astar.spi.SkipEdgeStrategy;

/**
* Skips Edges when the available battery distance of a vehicle is less than the accumulated driving
* distance of the same vehicle
*/
public class BatteryDistanceSkipEdgeStrategy<
State extends AStarState<State, Edge, ?>, Edge extends AStarEdge<State, Edge, ?>
>
implements SkipEdgeStrategy<State, Edge> {

private final Predicate<State> shouldSkip;

public BatteryDistanceSkipEdgeStrategy(Predicate<State> shouldSkip) {
this.shouldSkip = shouldSkip;
}

@Override
public boolean shouldSkipEdge(State current, Edge edge) {
return shouldSkip.test(current);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package org.opentripplanner.routing.impl;

import java.util.Optional;
import org.opentripplanner.street.search.state.State;

public class BatteryValidator {

public static boolean wouldBatteryRunOut(Object current) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way you can get away without the cast?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not find any way, without breaking the AStarArchitectureTest.
If I got it right, we would have to use a concrete State as Generic in our Strategy, which would break the AStarArchitectureTest.

double drivenBatteryMeters =
((org.opentripplanner.street.search.state.State) current).drivenBatteryMeters;
Optional<Double> currentRangeMeters = ((State) current).currentRangeMeters;
if (currentRangeMeters.isEmpty()) {
return false;
}
return currentRangeMeters.get() < drivenBatteryMeters;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import javax.annotation.Nullable;
import org.opentripplanner.astar.model.GraphPath;
import org.opentripplanner.astar.spi.TraverseVisitor;
import org.opentripplanner.astar.strategy.BatteryDistanceSkipEdgeStrategy;
import org.opentripplanner.astar.strategy.ComposingSkipEdgeStrategy;
import org.opentripplanner.astar.strategy.DurationSkipEdgeStrategy;
import org.opentripplanner.astar.strategy.PathComparator;
import org.opentripplanner.ext.dataoverlay.routing.DataOverlayContext;
Expand Down Expand Up @@ -88,8 +90,11 @@ public List<GraphPath<State, Edge, Vertex>> getPaths(
.of()
.setHeuristic(new EuclideanRemainingWeightHeuristic(maxCarSpeed))
.setSkipEdgeStrategy(
new DurationSkipEdgeStrategy(
preferences.maxDirectDuration().valueOf(request.journey().direct().mode())
new ComposingSkipEdgeStrategy<>(
new DurationSkipEdgeStrategy(
preferences.maxDirectDuration().valueOf(request.journey().direct().mode())
),
new BatteryDistanceSkipEdgeStrategy(BatteryValidator::wouldBatteryRunOut)
Copy link
Member

Choose a reason for hiding this comment

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

We only need this heuristic for rental searches. Can you only add it if there mode contains rental?

Copy link
Author

Choose a reason for hiding this comment

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

We added a method called getSkipEdgeStrategy where we check if the mode includes renting and call the BatterySkipEdgeStrategy if true

)
)
// FORCING the dominance function to weight only
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.opentripplanner.service.vehiclerental.model;

import java.time.Instant;
import java.util.Optional;
import java.util.Set;
import org.opentripplanner.framework.i18n.I18NString;
import org.opentripplanner.street.model.RentalFormFactor;
Expand Down Expand Up @@ -133,4 +134,8 @@ public VehicleRentalStationUris getRentalUris() {
public VehicleRentalSystem getVehicleRentalSystem() {
return system;
}

public Optional<Double> getCurrentRangeMeters() {
Copy link
Member

Choose a reason for hiding this comment

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

Use an OptionalDouble.

Copy link
Author

Choose a reason for hiding this comment

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

we just used a basic double because of this change: #5960 (comment)

return Optional.ofNullable(currentRangeMeters);
}
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package org.opentripplanner.service.vehiclerental.street;

import java.util.Collections;
import java.util.Optional;
import java.util.Set;
import javax.annotation.Nonnull;
import org.opentripplanner.framework.i18n.I18NString;
import org.opentripplanner.routing.api.request.StreetMode;
import org.opentripplanner.service.vehiclerental.model.VehicleRentalPlace;
import org.opentripplanner.service.vehiclerental.model.VehicleRentalVehicle;
import org.opentripplanner.street.model.RentalFormFactor;
import org.opentripplanner.street.model.edge.Edge;
import org.opentripplanner.street.search.state.State;
Expand All @@ -19,10 +21,12 @@
public class VehicleRentalEdge extends Edge {

public final RentalFormFactor formFactor;
private final VehicleRentalPlaceVertex vertex;

private VehicleRentalEdge(VehicleRentalPlaceVertex vertex, RentalFormFactor formFactor) {
super(vertex, vertex);
this.formFactor = formFactor;
this.vertex = vertex;
}

public static VehicleRentalEdge createVehicleRentalEdge(
Expand Down Expand Up @@ -169,6 +173,7 @@ public State[] traverse(State s0) {
? (int) preferences.pickupTime().toSeconds()
: (int) preferences.dropOffTime().toSeconds()
);
s1.setCurrentRangeMeters(getCurrentRangeMeters());
s1.setBackMode(null);
return s1.makeStateArray();
}
Expand Down Expand Up @@ -206,4 +211,11 @@ private static Set<RentalFormFactor> allowedModes(StreetMode streetMode) {
default -> Set.of();
};
}

public Optional<Double> getCurrentRangeMeters() {
if (vertex.getStation() instanceof VehicleRentalVehicle) {
return ((VehicleRentalVehicle) vertex.getStation()).getCurrentRangeMeters();
}
return Optional.empty();
Copy link
Member

Choose a reason for hiding this comment

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

Please move this into the domain model, ie. have a method on VehicleRentalPlace that encapsulates this.

Copy link
Author

Choose a reason for hiding this comment

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

added it to VehicleRentalPlace and were able to remove the if-check as we have implementations in each of the specific Edge-Types

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1183,6 +1183,10 @@ private StateEditor doTraverse(State s0, TraverseMode traverseMode, boolean walk
s1.incrementWalkDistance(getDistanceWithElevation());
}

if (traverseMode.isCyclingIsh() && s1.isVehicleRentable()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why only for cyling-ish? Cars have a range as well.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the check for cycling-ish

s1.incrementDrivenBatteryMeters(getDistanceWithElevation());
}

if (costExtension != null) {
weight += costExtension.calculateExtraCost(s0, length_mm, traverseMode);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;
import javax.annotation.Nonnull;
Expand Down Expand Up @@ -52,6 +53,12 @@ public class State implements AStarState<State, Edge, Vertex>, Cloneable {
// we should DEFINITELY rename this variable and the associated methods.
public double walkDistance;

// how far a sharing vehicle powered by battery has driven
public double drivenBatteryMeters;
Copy link
Member

Choose a reason for hiding this comment

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

Since bikes and scooters are ridden, I would call this variable travelledBatteryMeters or traversedBatteryMeters (more correct).

Copy link
Author

Choose a reason for hiding this comment

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

we chose traversedBatteryMeters as a new name, thanks


//the available battery distance of a currently selected sharing vehicle
public Optional<Double> currentRangeMeters;
Copy link
Member

Choose a reason for hiding this comment

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

For reasons that I don't quite understand it's totally unusual in Java to have instance fields that are Optionals. You have to use a regular double with a magic value for "no value".

Copy link
Member

Choose a reason for hiding this comment

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

It's probably also better for performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, would it be ok, to use Double(using null as "no value"), the wrapper class instead of the primitive double?
Because using a magic value for "no value" sounds slightly ugly.

Copy link
Member

Choose a reason for hiding this comment

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

I know that it's ugly but we do that quite a bit in OTP to get the absolute best performance. This is a very hot code path.

Copy link
Member

Choose a reason for hiding this comment

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

Also please make as many fields as possible private.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please make as many fields as possible private.

sounds good to me. We weren't sure here because the other fields in this class were public. But if you don't see a problem here, we'll change the added field.

Copy link
Contributor

@tobsesHub tobsesHub Jul 15, 2024

Choose a reason for hiding this comment

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

I know that it's ugly but we do that quite a bit in OTP to get the absolute best performance. This is a very hot code path.

Okay. What do you think would be a good value?
Zero would be really bad because if the battery was emty, we would ignore our logic and it would use that vehicle.
Would Double.MAX_VALUE or DOUBLE.MIN_VALUE be a good value?
Or maybe a negative number, then we would have to change the check with the negative number.

Copy link
Author

Choose a reason for hiding this comment

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

We made it a basic double and used Double.POSITIVE_INFINITY as the initializing value


Copy link
Contributor

Choose a reason for hiding this comment

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

As said in the developer meeting, I agree that the algorithm itself has to be evaluated firstmost.

With that said, you can save a considerable amount of memory by selecting different data types for the fields.
By decreasing the granularity with three bits (8 meters) you can represent values up to 2040 meters using a single byte. The values are still comparable, it's just that the unit is octa-meters.

The value has to be mangled (shifted and casted) to fit but those operations are basically free from a CPU perspective.

Copy link
Member

Choose a reason for hiding this comment

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

In practice it is not so easy, but lets not go into discussion on HW and Java architecture before we know how to solve this. I have a strong feeling that we do not need to add anything to the state to solve this.

/* CONSTRUCTORS */

/**
Expand Down Expand Up @@ -82,6 +89,8 @@ public State(
vertex.rentalRestrictions().noDropOffNetworks();
}
this.walkDistance = 0;
this.drivenBatteryMeters = 0;
this.currentRangeMeters = Optional.empty();
this.time = startTime.getEpochSecond();
}

Expand Down Expand Up @@ -360,6 +369,8 @@ public State reverse() {
editor.incrementTimeInSeconds(orig.getAbsTimeDeltaSeconds());
editor.incrementWeight(orig.getWeightDelta());
editor.incrementWalkDistance(orig.getWalkDistanceDelta());
editor.incrementDrivenBatteryMeters(orig.getBatteryDistanceDelta());
editor.setCurrentRangeMeters(orig.getCurrentRangeMeters());

// propagate the modes through to the reversed edge
editor.setBackMode(orig.getBackMode());
Expand Down Expand Up @@ -503,6 +514,22 @@ private double getWalkDistanceDelta() {
}
}

private double getBatteryDistanceDelta() {
if (backState != null) {
return Math.abs(this.drivenBatteryMeters - backState.drivenBatteryMeters);
} else {
return 0.0;
}
}

private Optional<Double> getCurrentRangeMeters() {
if (backState != null) {
return backState.currentRangeMeters;
} else {
return Optional.empty();
}
}

private State reversedClone() {
StreetSearchRequest reversedRequest = request
.copyOfReversed(getTime())
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.opentripplanner.street.search.state;

import java.util.Optional;
import java.util.Set;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -196,6 +197,19 @@ public void incrementWalkDistance(double length) {
child.walkDistance += length;
}

public void incrementDrivenBatteryMeters(double length) {
if (length < 0) {
LOG.warn("A state's battery distance is being incremented by a negative amount.");
Copy link
Member

Choose a reason for hiding this comment

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

I will have to check with the other developers but I would be totally unforgiving and throw an exception in this case. This indicates a bug elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

The incrementWalkDistance, incrementTimeInSeconds, incrementWeight methods in the StateEditor class do it the same way(thwowing no exception. So probably, if we change this, we should change those methods too.

We could add exceptions like NegativeBatteryMeterIncrement, NegativeWalkDistanceIncrement...

But we don' t know really good the exception handling in OTP.
So at least, I would prefer to create a new PR for that, if we should change that.

defectiveTraversal = true;
return;
}
child.drivenBatteryMeters += length;
}

public void setCurrentRangeMeters(Optional<Double> currentRangeMeters) {
child.currentRangeMeters = currentRangeMeters;
}

/* Basic Setters */

public void resetEnteredNoThroughTrafficArea() {
Expand Down Expand Up @@ -389,4 +403,8 @@ private void cloneStateDataAsNeeded() {
if (child.backState != null && child.stateData == child.backState.stateData) child.stateData =
child.stateData.clone();
}

public boolean isVehicleRentable() {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have a different name from the delegate method?

Copy link
Author

Choose a reason for hiding this comment

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

We changed it to isRentableVehicle, as the delegate is called

return child.isRentingVehicle();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package org.opentripplanner.astar.strategy;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.Optional;
import org.junit.jupiter.api.Test;
import org.opentripplanner.routing.algorithm.GraphRoutingTest;
import org.opentripplanner.routing.impl.BatteryValidator;
import org.opentripplanner.street.search.state.State;
import org.opentripplanner.street.search.state.TestStateBuilder;

public class BatteryDistanceSkipEdgeStrategyTest extends GraphRoutingTest {

/**
* battery is not enough for driven distance-> skips Edge
* battery is 0m, driven meters = 100 => true
*/
@Test
void batteryIsNotEnough() {
var state = getState(100.0);

state.currentRangeMeters = Optional.of(0.0);

var strategy = new BatteryDistanceSkipEdgeStrategy(BatteryValidator::wouldBatteryRunOut);

assertTrue(strategy.shouldSkipEdge(state, null));
}

/**
* battery is enough for driven distance-> does not skip Edge
* battery is 4000m, driven meters = 100 => false
*/
@Test
void batteryIsEnough() {
var state = getState(100.0);
state.currentRangeMeters = Optional.of(4000.0);

var strategy = new BatteryDistanceSkipEdgeStrategy(BatteryValidator::wouldBatteryRunOut);
assertFalse(strategy.shouldSkipEdge(state, null));
}

/**
* battery dies at exact time location is reached -> does not skip edge
* battery is 100m, driven meters = 100 => false
*/
@Test
void batteryDiesAtFinalLocation() {
var state = getState(100.0);

state.currentRangeMeters = Optional.of(100.0);

var strategy = new BatteryDistanceSkipEdgeStrategy(BatteryValidator::wouldBatteryRunOut);
assertFalse(strategy.shouldSkipEdge(state, null));
}

/**
* battery has remaining Energy, no distance was driven so far -> does not skip edge
* battery is 100m, driven meters = 0 => false
*/
@Test
void noDrivenMeters() {
var state = getState(0.0);
state.currentRangeMeters = Optional.of(100.0);

var strategy = new BatteryDistanceSkipEdgeStrategy(BatteryValidator::wouldBatteryRunOut);
assertFalse(strategy.shouldSkipEdge(state, null));
}

/**
* vehicle.currentRangeMeters (battery) is Null or of Empty Value -> does not skip Edge
* Battery is Optional.Empty() => false
*/
@Test
void batteryHasNoValue() {
var state = TestStateBuilder.ofScooterRental().build();
state.currentRangeMeters = Optional.empty();

var strategy = new BatteryDistanceSkipEdgeStrategy(BatteryValidator::wouldBatteryRunOut);
assertFalse(strategy.shouldSkipEdge(state, null));
}

private static State getState(double drivenBatteryMeters) {
var state = TestStateBuilder.ofScooterRental().build();
state.drivenBatteryMeters = drivenBatteryMeters;
return state;
}
}
3 changes: 2 additions & 1 deletion test/performance/norway/travelSearch-expected-results.csv
Original file line number Diff line number Diff line change
Expand Up @@ -149,4 +149,5 @@ tcId,nTransfers,duration,cost,walkDistance,startTime,endTime,agencies,modes,rout
239,0,00:42:00,7360,994,10:00:00,10:42:00,,,,,Walk 13m37s ~ YTI:Vehicle:de703d2ea4b03d6af6b8837fd90e5e5ef95f3b9490c194fc8617924314c5427d Rental 28m23s
240,0,00:50:54,8937,994,10:00:00,10:50:54,,,,,Walk 13m37s ~ YTI:Vehicle:de703d2ea4b03d6af6b8837fd90e5e5ef95f3b9490c194fc8617924314c5427d Rental 37m17s
241,0,00:46:40,7954,994,10:00:00,10:46:40,,,,,Walk 13m37s ~ YTI:Vehicle:de703d2ea4b03d6af6b8837fd90e5e5ef95f3b9490c194fc8617924314c5427d Rental 33m3s
242,0,01:03:31,9788,994,10:00:00,11:03:31,,,,,Walk 13m37s ~ YTI:Vehicle:de703d2ea4b03d6af6b8837fd90e5e5ef95f3b9490c194fc8617924314c5427d Rental 49m54s
242,0,01:03:31,9788,994,10:00:00,11:03:31,,,,,Walk 13m37s ~ YTI:Vehicle:de703d2ea4b03d6af6b8837fd90e5e5ef95f3b9490c194fc8617924314c5427d Rental 49m54s
243,0,2:03:31,13000,13000,10:00:00,12:03:31,,,,,Walk 2h03m31s
1 change: 1 addition & 0 deletions test/performance/norway/travelSearch.csv
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,4 @@ testCaseId,description,departure,arrival,window,origin,fromPlace,fromLat,fromLon
240,Eiksmarka to Kvadraturen,10:00,,,Eiksmarka,,59.9463,10.6263,Kvadraturen,,59.9109,10.7423,scooter-rent,WALK|SCOOTER_RENT
241,Eiksmarka to Bergskogen,10:00,,,Eiksmarka,,59.9463,10.6263,Bergskogen,,59.94770,10.74223,scooter-rent,WALK|SCOOTER_RENT
242,Eiksmarka to Refstad,10:00,,,Eiksmarka,,59.9463,10.6263,Refstad,,59.9388,10.8012,scooter-rent,WALK|SCOOTER_RENT
243,Oslo to Ekeberg,10:00,,,Oslo,,59.917696,10.735079,Ekeberg,,59.8896,10.79424,scooter-rent,WALK|SCOOTER_RENT
Loading