-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: dev-2.x
Are you sure you want to change the base?
Changes from all commits
d204d3e
7824840
954c3d8
f1e354b
d980fa2
6cd22f6
f591224
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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,16 @@ | ||
package org.opentripplanner.routing.impl; | ||
|
||
import org.opentripplanner.street.search.state.State; | ||
|
||
public class BatteryValidator { | ||
|
||
public static boolean wouldBatteryRunOut(Object current) { | ||
State state = (State) current; | ||
double traversedBatteryMeters = state.traversedBatteryMeters; | ||
double currentRangeMeters = state.currentRangeMeters; | ||
if (currentRangeMeters == Double.POSITIVE_INFINITY) { | ||
return false; | ||
} | ||
return currentRangeMeters < traversedBatteryMeters; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,13 @@ 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 traversedBatteryMeters; | ||
|
||
//the available battery distance of a currently selected sharing vehicle | ||
//setting a magic value of 0.0 to make sure | ||
public double currentRangeMeters; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. The value has to be mangled (shifted and casted) to fit but those operations are basically free from a CPU perspective. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 */ | ||
|
||
/** | ||
|
@@ -82,6 +89,8 @@ public State( | |
vertex.rentalRestrictions().noDropOffNetworks(); | ||
} | ||
this.walkDistance = 0; | ||
this.traversedBatteryMeters = 0; | ||
this.currentRangeMeters = Double.POSITIVE_INFINITY; | ||
this.time = startTime.getEpochSecond(); | ||
} | ||
|
||
|
@@ -360,6 +369,8 @@ public State reverse() { | |
editor.incrementTimeInSeconds(orig.getAbsTimeDeltaSeconds()); | ||
editor.incrementWeight(orig.getWeightDelta()); | ||
editor.incrementWalkDistance(orig.getWalkDistanceDelta()); | ||
editor.incrementTraversedBatteryMeters(orig.getBatteryDistanceDelta()); | ||
editor.setCurrentRangeMeters(orig.getCurrentRangeMeters()); | ||
|
||
// propagate the modes through to the reversed edge | ||
editor.setBackMode(orig.getBackMode()); | ||
|
@@ -503,6 +514,22 @@ private double getWalkDistanceDelta() { | |
} | ||
} | ||
|
||
private double getBatteryDistanceDelta() { | ||
if (backState != null) { | ||
return Math.abs(this.traversedBatteryMeters - backState.traversedBatteryMeters); | ||
} else { | ||
return 0.0; | ||
} | ||
} | ||
|
||
private double getCurrentRangeMeters() { | ||
if (backState != null) { | ||
return backState.currentRangeMeters; | ||
} else { | ||
return Double.POSITIVE_INFINITY; | ||
} | ||
} | ||
|
||
private State reversedClone() { | ||
StreetSearchRequest reversedRequest = request | ||
.copyOfReversed(getTime()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -196,6 +196,19 @@ public void incrementWalkDistance(double length) { | |
child.walkDistance += length; | ||
} | ||
|
||
public void incrementTraversedBatteryMeters(double length) { | ||
if (length < 0) { | ||
LOG.warn("A state's battery distance is being incremented by a negative amount."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The We could add exceptions like NegativeBatteryMeterIncrement, NegativeWalkDistanceIncrement... But we don' t know really good the exception handling in OTP. |
||
defectiveTraversal = true; | ||
return; | ||
} | ||
child.traversedBatteryMeters += length; | ||
} | ||
|
||
public void setCurrentRangeMeters(double currentRangeMeters) { | ||
child.currentRangeMeters = currentRangeMeters; | ||
} | ||
|
||
/* Basic Setters */ | ||
|
||
public void resetEnteredNoThroughTrafficArea() { | ||
|
@@ -389,4 +402,8 @@ private void cloneStateDataAsNeeded() { | |
if (child.backState != null && child.stateData == child.backState.stateData) child.stateData = | ||
child.stateData.clone(); | ||
} | ||
|
||
public boolean isRentingVehicle() { | ||
return child.isRentingVehicle(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
package org.opentripplanner.astar.strategy; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertFalse; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
||
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 = 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 = 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 = 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 = 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 = Double.POSITIVE_INFINITY; | ||
|
||
var strategy = new BatteryDistanceSkipEdgeStrategy(BatteryValidator::wouldBatteryRunOut); | ||
assertFalse(strategy.shouldSkipEdge(state, null)); | ||
} | ||
|
||
private static State getState(double traversedBatteryMeters) { | ||
var state = TestStateBuilder.ofScooterRental().build(); | ||
state.traversedBatteryMeters = traversedBatteryMeters; | ||
return state; | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.