Take rental vehicle battery level into account #5960

UserX20 · 2024-07-09T14:55:10Z

Summary

If a vehicle, for example an E-scooter, was to be rented it was possible for this vehicle to have had not enough remaining power to complete the entire trip.
In this case you would probably want to use a different scooter, which has enough remaining energy.
This PR adds this feature.

Added a Skip Edge Strategy which we call in the A-Star Routing

Issue

#5959

Unit tests

added a unit test for BatteryDistanceSkipEdgeStrategy

Documentation

N/A

Bumping the serialization version id

We're unsure if it needs to be bumped. added private final VehicleRentalPlaceVertex vertex; to VehicleRentalEdge
also added fields to State

Coordinates of a Scooter with currentRangeMeters (battery) = 0.0

leonardehrenfried · 2024-07-11T14:26:54Z

src/main/java/org/opentripplanner/routing/impl/BatteryValidator.java

+
+public class BatteryValidator {
+
+  public static boolean wouldBatteryRunOut(Object current) {


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

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.

leonardehrenfried · 2024-07-11T14:37:51Z

src/main/java/org/opentripplanner/routing/impl/GraphPathFinder.java

+          new DurationSkipEdgeStrategy(
+            preferences.maxDirectDuration().valueOf(request.journey().direct().mode())
+          ),
+          new BatteryDistanceSkipEdgeStrategy(BatteryValidator::wouldBatteryRunOut)


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

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

leonardehrenfried · 2024-07-11T14:39:22Z

src/main/java/org/opentripplanner/service/vehiclerental/model/VehicleRentalVehicle.java

@@ -133,4 +134,8 @@ public VehicleRentalStationUris getRentalUris() {
  public VehicleRentalSystem getVehicleRentalSystem() {
    return system;
  }
+
+  public Optional<Double> getCurrentRangeMeters() {


Use an OptionalDouble.

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

leonardehrenfried · 2024-07-11T14:43:25Z

src/main/java/org/opentripplanner/street/search/state/State.java

@@ -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;


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

we chose traversedBatteryMeters as a new name, thanks

leonardehrenfried · 2024-07-11T14:44:51Z

src/main/java/org/opentripplanner/street/search/state/State.java

+  public double drivenBatteryMeters;
+
+  //the available battery distance of a currently selected sharing vehicle
+  public Optional<Double> currentRangeMeters;


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".

It's probably also better for performance.

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.

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.

Also please make as many fields as possible private.

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.

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.

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

leonardehrenfried · 2024-07-11T14:47:46Z

src/main/java/org/opentripplanner/street/search/state/StateEditor.java

@@ -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.");


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.

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.

leonardehrenfried · 2024-07-11T14:49:53Z

src/main/java/org/opentripplanner/service/vehiclerental/street/VehicleRentalEdge.java

+    if (vertex.getStation() instanceof VehicleRentalVehicle) {
+      return ((VehicleRentalVehicle) vertex.getStation()).getCurrentRangeMeters();
+    }
+    return Optional.empty();


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

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

leonardehrenfried · 2024-07-11T14:50:30Z

src/main/java/org/opentripplanner/street/model/edge/StreetEdge.java

@@ -1183,6 +1183,10 @@ private StateEditor doTraverse(State s0, TraverseMode traverseMode, boolean walk
      s1.incrementWalkDistance(getDistanceWithElevation());
    }

+    if (traverseMode.isCyclingIsh() && s1.isVehicleRentable()) {


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

Removed the check for cycling-ish

leonardehrenfried · 2024-07-11T14:51:16Z

src/main/java/org/opentripplanner/street/search/state/StateEditor.java

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


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

We changed it to isRentableVehicle, as the delegate is called

codecov · 2024-07-11T16:13:51Z

Codecov Report

Attention: Patch coverage is 76.31579% with 9 lines in your changes missing coverage. Please review.

Project coverage is 69.53%. Comparing base (20fc8ce) to head (f1e354b).
Report is 27 commits behind head on dev-2.x.

Files	Patch %	Lines
...org/opentripplanner/street/search/state/State.java	60.00%	2 Missing and 2 partials ⚠️
...entripplanner/street/search/state/StateEditor.java	55.55%	3 Missing and 1 partial ⚠️
...opentripplanner/routing/impl/BatteryValidator.java	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             dev-2.x    #5960   +/-   ##
==========================================
  Coverage      69.53%   69.53%           
- Complexity     17111    17129   +18     
==========================================
  Files           1938     1940    +2     
  Lines          73774    73810   +36     
  Branches        7548     7551    +3     
==========================================
+ Hits           51296    51327   +31     
- Misses         19840    19844    +4     
- Partials        2638     2639    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

leonardehrenfried · 2024-07-11T16:30:35Z

Also please add tests for the edges that you change. Take a look at how it's done in StreetEdgeGeofencingTest.

UserX20 · 2024-07-31T15:17:31Z

Also please add tests for the edges that you change. Take a look at how it's done in StreetEdgeGeofencingTest.

We have added to the StateEditorTest and created a new StateEdgeTest, as we only change the state within an edge

t2gran · 2024-08-12T14:09:13Z

Sorry, I would like to see a suggestion for the strategy on how to implement this, before starting to look at the code. I made a comment to the issue for ideas on how to implement it.

jtorin · 2024-08-27T10:06:55Z

src/main/java/org/opentripplanner/street/search/state/State.java

+  //the available battery distance of a currently selected sharing vehicle
+  //setting a magic value of 0.0 to make sure
+  public double currentRangeMeters;
+


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.

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.

t2gran · 2024-08-28T10:13:51Z

There is no point in reviewing this before we agree on the algorithm/analysis/design - se discussion in issue #5959.

UserX20 added 3 commits July 9, 2024 15:51

feat: use battery status (currentRangeMeters) in Vehicle Routing

d204d3e

test: BatteryDistanceSkipEdgeStrategyTest

7824840

test: added testcase to speedtest

954c3d8

Coordinates of a Scooter with currentRangeMeters (battery) = 0.0

UserX20 requested a review from a team as a code owner July 9, 2024 14:55

fix: architecture and use currentRangeMeters via State

f1e354b

UserX20 force-pushed the feat/max-battery-distance-rented-vehicles branch from 758ffe8 to f1e354b Compare July 10, 2024 11:06

leonardehrenfried requested review from leonardehrenfried and optionsome July 11, 2024 13:35

leonardehrenfried changed the title ~~Feat/max battery distance rented vehicles~~ Take battery level into account for rental vehicles Jul 11, 2024

leonardehrenfried added New Feature Vehicle Rental labels Jul 11, 2024

leonardehrenfried reviewed Jul 11, 2024

View reviewed changes

leonardehrenfried changed the title ~~Take battery level into account for rental vehicles~~ Take rental vehicle battery level into account for rental Jul 11, 2024

leonardehrenfried changed the title ~~Take rental vehicle battery level into account for rental~~ Take rental vehicle battery level into account Jul 11, 2024

t2gran added this to the 2.6 (next release) milestone Jul 23, 2024

UserX20 added 2 commits July 31, 2024 17:09

refactor: refactoring multiple things

d980fa2

test: StateEditorTest and Test for changes to an edges state

6cd22f6

UserX20 force-pushed the feat/max-battery-distance-rented-vehicles branch from fe612b8 to 6cd22f6 Compare July 31, 2024 15:14

refactor: added a nullcheck to getCurrentRangeMeters()

f591224

jtorin reviewed Aug 27, 2024

View reviewed changes

t2gran modified the milestones: 2.6, 2.7 (next release) Sep 18, 2024

habrahamsson-skanetrafiken marked this pull request as draft October 15, 2024 08:43

optionsome removed their request for review December 31, 2024 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take rental vehicle battery level into account #5960

Take rental vehicle battery level into account #5960

UserX20 commented Jul 9, 2024

leonardehrenfried Jul 11, 2024

tobsesHub Jul 30, 2024

leonardehrenfried Jul 11, 2024

UserX20 Jul 31, 2024

leonardehrenfried Jul 11, 2024

UserX20 Jul 31, 2024

leonardehrenfried Jul 11, 2024

UserX20 Jul 31, 2024

leonardehrenfried Jul 11, 2024

leonardehrenfried Jul 11, 2024

tobsesHub Jul 15, 2024

leonardehrenfried Jul 15, 2024

leonardehrenfried Jul 15, 2024

tobsesHub Jul 15, 2024

tobsesHub Jul 15, 2024 •

edited

Loading

UserX20 Jul 31, 2024

leonardehrenfried Jul 11, 2024

tobsesHub Jul 30, 2024

leonardehrenfried Jul 11, 2024

UserX20 Jul 31, 2024

leonardehrenfried Jul 11, 2024

UserX20 Jul 31, 2024

leonardehrenfried Jul 11, 2024

UserX20 Jul 31, 2024

codecov bot commented Jul 11, 2024

leonardehrenfried commented Jul 11, 2024

UserX20 commented Jul 31, 2024

t2gran commented Aug 12, 2024 •

edited

Loading

jtorin Aug 27, 2024

t2gran Aug 28, 2024

t2gran commented Aug 28, 2024


		public class BatteryValidator {

		public static boolean wouldBatteryRunOut(Object current) {

Take rental vehicle battery level into account #5960

Are you sure you want to change the base?

Take rental vehicle battery level into account #5960

Conversation

UserX20 commented Jul 9, 2024

Summary

Issue

Unit tests

Documentation

Bumping the serialization version id

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobsesHub Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2024

Codecov Report

leonardehrenfried commented Jul 11, 2024

UserX20 commented Jul 31, 2024

t2gran commented Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2gran commented Aug 28, 2024

tobsesHub Jul 15, 2024 •

edited

Loading

t2gran commented Aug 12, 2024 •

edited

Loading