Create own parking preferences for bike and car in the internal model #5521

optionsome · 2023-11-21T08:26:35Z

Summary

This refactors parking preferences so that in the internal model the same preferences are not used for all types of parking but it is now possible to set them different for car and bike parking. This pr is done in preparation to #5185

This also removes support for useVehicleParkingAvailabilityInformation that was only usable through REST API

Issue

This is related to #4803

Unit tests

Updated and added tests.

Documentation

Minor automatic updates.

Changelog

From title

optionsome · 2023-11-21T08:27:26Z

src/main/java/org/opentripplanner/routing/api/request/preference/VehicleParkingPreferences.java

+  public Duration parkTime() {
+    return parkTime;
+  }
+
+  /** Cost of parking a bike. */
+  public Cost parkCost() {
+    return parkCost;
+  }


Should we expose (or even use) Cost and Duration?

The preferences is part of the API - the purpose is to make it easy to use and safe - avoid mistakes. So, indeed we should use the type-safe wrapper types. The API is not about the internal model. When we do routing in AStar and Raptor we should copy/map the values into the AStar Request(does not exist) and into the Raptor Request. The Raptor and AStar requests should be consered with performance. In this way we make a nice user friendly API witch should hide the implementation details from the APIs. We can reuse types and structures - the mapping into Raptor and AStar can use the same source (walkReluctance, boardingCost ...).

Short: Use the typesafe version of Cost and Reluctance here.

I agree. Whatever call site needs something more efficient needs to create their own model.

optionsome · 2023-11-21T08:29:01Z

src/main/java/org/opentripplanner/standalone/config/routerequest/RouteRequestConfig.java

+              .of("unpreferredVehicleParkingTagCost")
+              .since(V2_3)
+              .summary("What cost to add if a parking facility doesn't contain a preferred tag.")
+              .description("See `preferredVehicleParkingTags`.")
+              .asInt(VehicleParkingPreferences.DEFAULT.unpreferredVehicleParkingTagCost().toSeconds())
+          )
+          .withBannedVehicleParkingTags(
+            c
+              .of("bannedVehicleParkingTags")
+              .since(V2_1)
+              .summary(
+                "Tags with which a vehicle parking will not be used. If empty, no tags are banned."
+              )
+              .asStringSet(List.of())
+          )
+          .withRequiredVehicleParkingTags(
+            c
+              .of("requiredVehicleParkingTags")
+              .since(V2_1)
+              .summary(
+                "Tags without which a vehicle parking will not be used. If empty, no tags are required."
+              )
+              .asStringSet(List.of())
+          )


Should we remove these and instead create versions of these that are specific to car and bike?

I think that is the correct way to do it. Remember that if you use the same type within each mode then you only need to write one config for it as well.

Q: Tag is very generic, it does not say witch context it comes from. Is it a network, a OSM tag, or tag from somewhere else?

I think I'll postpone the router-config refactoring to a follow up pr. Otherwise I would need to do it in two phases and this is a new feature anyway that no one is wanting to use yet.

The tag can originate from different places depending on the origin of the parking. I think mostly it's generated off some OSM tags but in Finland we load the tags from a custom format from a realtime updater. I think the network/feed is never a tag.

Thank you for the explanation, a hint on this in the doc would be useful:

The tag can originate from different places depending on the origin of the parking(OSM or RT feed).

codecov · 2023-11-21T08:43:41Z

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (c706466) 66.93% compared to head (9569d93) 67.18%.
Report is 208 commits behind head on dev-2.x.

❗ Current head 9569d93 differs from pull request most recent head 3d98683. Consider uploading reports for the commit 3d98683 to get more accurate results

Files	Patch %	Lines
...equest/preference/filter/VehicleParkingFilter.java	86.20%	2 Missing and 2 partials ⚠️
.../request/preference/VehicleParkingPreferences.java	96.20%	0 Missing and 3 partials ⚠️
...ain/java/org/opentripplanner/model/plan/Place.java	0.00%	2 Missing ⚠️
...routing/api/request/preference/CarPreferences.java	88.88%	0 Missing and 1 partial ⚠️
...lanner/routing/vehicle_parking/VehicleParking.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5521      +/-   ##
=============================================
+ Coverage      66.93%   67.18%   +0.25%     
- Complexity     15700    16106     +406     
=============================================
  Files           1819     1856      +37     
  Lines          70248    71391    +1143     
  Branches        7392     7474      +82     
=============================================
+ Hits           47022    47967     +945     
- Misses         20772    20922     +150     
- Partials        2454     2502      +48

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

src/main/java/org/opentripplanner/api/common/RoutingResource.java

src/test/java/org/opentripplanner/apis/gtfs/mapping/RouteRequestMapperTest.java

leonardehrenfried · 2023-11-21T11:39:47Z

src/main/java/org/opentripplanner/standalone/config/routerequest/RouteRequestConfig.java

+      .withParking(it ->
+        it
+          .withUnpreferredVehicleParkingTagCost(
+            c


The first 3 items are identical with the ones below. Can you make them bike/car specific or extract a re-usable method?

I think I'll create a reusable method if possible, I'll check.

I think I'll do it in a follow up pr because some of the parameter names are not generic now (they will be in in the future).

leonardehrenfried · 2023-11-21T11:41:21Z

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

+      ? s0.getRequest().preferences().car().parking()
+      : s0.getRequest().preferences().bike().parking();


Can you create a utility method for this somewhere in the preferences?

I thought about it too but wasn't sure where to put it. I'll try to find a place.

I created a method under the root preferences.

As I mentioned on todays meeting, we are passing in the API request/preferences into AStar without a mapping it. One of the effects is that we then need to do mapping everywhere it is used instead of in one place - I do not expect you tro refactor this, but I just wanted to highlight the pain. If it is interesting, we could take one feature like the parking and refactor AStar with just that. Then the job is not so big, and it can be done in iterations. Also, when a pattern is established it become easier to take do the rest - just follow the recipe. In this case the AStar "preference" should have a method to return a AStar specific VehiclePariking based on the s0.currentMode().

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

leonardehrenfried

I have a few requests. Most importantly, please bring back the test for the GraphQL request mapper.

t2gran · 2023-11-21T18:53:54Z

src/main/java/org/opentripplanner/framework/tostring/ToStringBuilder.java

+  public ToStringBuilder addBool(String name, Boolean value, Boolean ignoreValue) {
+    return addIfNotIgnored(name, value, ignoreValue, Object::toString);
+  }
+


I prefer the addBoolIfTrue over this. For example useAvailabilityInformation=false is not as readable as skipAvailabilityInfo. We try to keep toStrings short and readable. You would need to repeat the default value in the toString inside the class to do this, but I prefer that for the extra readability.

Consider adding JavaDoc recommending the addBoolIfTrue or converting the usage of this. Note! That the addBoolIfTrue fits well with things that are off by default, and not so good with things that are on. Note also that the consequences of flipping the value in the toString is very small - it is just confusing.

This is not important!

I ended up renaming the parameter ignoreRealtimeAvailability. I was considering using the excludeRealtimeUpdates naming we used the plan pr but this paramete doesn't really exclude all realtime updates as the parks itself are added through realtime updates, it just ignores the realtime availability. I only did the renaming in the preferences, the REST API parameter stays as is.

t2gran · 2023-11-21T18:58:19Z

src/main/java/org/opentripplanner/routing/api/request/preference/RoutingPreferences.java

+  /**
+   * Get parking preferences for the traverse mode. Note, only car and bike are supported.
+   */
+  public VehicleParkingPreferences parking(TraverseMode mode) {


Note! The TraverseMode is not part of the api, and using it here create a circular dependency - this is nothing new, so I am not requesting you to change it - but this is adding a string to the spagetti.

t2gran · 2023-11-21T19:13:26Z

...java/org/opentripplanner/routing/api/request/request/filter/VehicleParkingFilterRequest.java

@@ -11,7 +12,7 @@
 * A request object that checks if parking faclities match certain conditions for
 * inclusion/exclusion or preference/unpreference.
 */
-public class VehicleParkingFilterRequest {
+public class VehicleParkingFilterRequest implements Serializable {


FYI: I plan to split the filter logic from the select parameters/data. The current filters can not be reused in different places because it depends on the internal model - it is applied to one set of model classes, but in other use-cases we would like to use the same filters to filter other things. E.g. not filter Scheduled trips, but FLEX or transit-priority-groups.

This PR is ok, it is neutral when it comes to moving in a direction here. The select()/not() is good, but the returned Filter is not.

src/test/java/org/opentripplanner/framework/tostring/ToStringBuilderTest.java

...t/java/org/opentripplanner/routing/api/request/preference/VehicleParkingPreferencesTest.java

t2gran

It mostly looks OK, some comments and "frustrations", but your work is good.

t2gran · 2023-11-21T19:32:13Z

src/main/java/org/opentripplanner/routing/api/request/preference/VehicleParkingPreferences.java

+   * Parking containing select filters must only be usable and parking containing with not filters
+   * cannot be used.
+   */
+  public VehicleParkingFilterRequest filter() {


This is no longer part of the request, so it need a new name. I suggest just dropping the Request, and we will deal with the naming when splitting the data from the filtering implementation.

Issue is that this class contains VehicleParkingFilter objects already so can't use that name unless we first rename it. I couldn't really think of a good name for this. We can come up with a solution in the dev meeting.

optionsome · 2023-11-23T16:00:22Z

This is now ready for review again

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

leonardehrenfried

Please have a look at my request.

optionsome · 2023-11-27T08:25:08Z

I realized I have made a mistake here on the realtime usage. The default used to be the opposite of what it is now. I've converted this to be draft so we can discuss what to do with this next week.

optionsome · 2023-12-11T14:11:55Z

I've removed support for useVehicleParkingAvailabilityInformation as it was a REST API only feature and it's usage is a bit questionable as the actual parking usage is often way after the current moment and the availability can change. At some point, we might want to support a time based setting to use realtime availability in routing until a point in time if the parking actually happens within that time window.

optionsome · 2023-12-11T14:12:21Z

There was also an issue with copying over preferences when doing remodification. I've fixed that too.

optionsome added 4 commits November 20, 2023 13:46

Support default boolean value ignore in toStringBuilder

2a6522e

Refactor so that there are own parking preferences for bike and car

8ddc392

Move park time/cost to be under parking preferences

a90722f

Renaming to better follow conventions

a4a8819

optionsome added Improvement bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Config Change labels Nov 21, 2023

optionsome requested a review from a team as a code owner November 21, 2023 08:26

optionsome commented Nov 21, 2023

View reviewed changes

optionsome added 2 commits November 21, 2023 10:36

Fix test

cb0643f

Fix formatting

09835d6

leonardehrenfried requested review from leonardehrenfried and t2gran November 21, 2023 10:04

Rename test method

df56e77

leonardehrenfried reviewed Nov 21, 2023

View reviewed changes

src/main/java/org/opentripplanner/api/common/RoutingResource.java Show resolved Hide resolved

leonardehrenfried reviewed Nov 21, 2023

View reviewed changes

src/test/java/org/opentripplanner/apis/gtfs/mapping/RouteRequestMapperTest.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Nov 21, 2023

View reviewed changes

src/main/java/org/opentripplanner/street/model/edge/VehicleParkingEdge.java Outdated Show resolved Hide resolved

leonardehrenfried requested changes Nov 21, 2023

View reviewed changes

optionsome added 2 commits November 21, 2023 14:41

Minor refactoring

cb5b04b

Add back tests for the GTFS API

5ee913e

optionsome requested a review from leonardehrenfried November 21, 2023 12:48

t2gran reviewed Nov 21, 2023

View reviewed changes

src/test/java/org/opentripplanner/framework/tostring/ToStringBuilderTest.java Outdated Show resolved Hide resolved

t2gran reviewed Nov 21, 2023

View reviewed changes

...t/java/org/opentripplanner/routing/api/request/preference/VehicleParkingPreferencesTest.java Show resolved Hide resolved

t2gran requested changes Nov 21, 2023

View reviewed changes

optionsome added 2 commits November 23, 2023 17:56

Fix realtime check

a9af343

Update tests

19d3a3b

optionsome requested a review from t2gran November 23, 2023 15:59

Add descriptions for tags

cb3e273

leonardehrenfried reviewed Nov 24, 2023

View reviewed changes

src/main/java/org/opentripplanner/street/model/edge/VehicleParkingEdge.java Outdated Show resolved Hide resolved

leonardehrenfried requested changes Nov 24, 2023

View reviewed changes

Change params

9569d93

leonardehrenfried previously approved these changes Nov 24, 2023

View reviewed changes

optionsome marked this pull request as draft November 27, 2023 08:23

optionsome added 2 commits December 11, 2023 15:45

Remove support for using realtime availability in parking routing

2d540f8

Fix preference modification

3d98683

optionsome dismissed leonardehrenfried’s stale review via 3d98683 December 11, 2023 14:08

optionsome marked this pull request as ready for review December 11, 2023 14:09

optionsome requested a review from leonardehrenfried December 11, 2023 14:12

optionsome mentioned this pull request Dec 11, 2023

Create own rental preferences for bike and car in the internal model #5562

Merged

leonardehrenfried approved these changes Dec 12, 2023

View reviewed changes

t2gran approved these changes Dec 15, 2023

View reviewed changes

optionsome merged commit 942c155 into opentripplanner:dev-2.x Dec 15, 2023
5 checks passed

optionsome deleted the parking-preference-refactor branch December 15, 2023 18:53

t2gran pushed a commit that referenced this pull request Dec 15, 2023

Add changelog entry for #5521 [ci skip]

0f64965

t2gran pushed a commit that referenced this pull request Dec 15, 2023

Bump serialization version id for #5521

df9e1e1

t2gran added this to the 2.5 (next release) milestone Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create own parking preferences for bike and car in the internal model #5521

Create own parking preferences for bike and car in the internal model #5521

optionsome commented Nov 21, 2023 •

edited

Loading

optionsome Nov 21, 2023

t2gran Nov 21, 2023

leonardehrenfried Nov 21, 2023

optionsome Nov 21, 2023

t2gran Nov 21, 2023

optionsome Nov 21, 2023

t2gran Nov 21, 2023

codecov bot commented Nov 21, 2023 •

edited

Loading

leonardehrenfried Nov 21, 2023

optionsome Nov 21, 2023

optionsome Nov 21, 2023

leonardehrenfried Nov 21, 2023

optionsome Nov 21, 2023

optionsome Nov 21, 2023

t2gran Nov 21, 2023 •

edited

Loading

leonardehrenfried left a comment

t2gran Nov 21, 2023

t2gran Nov 21, 2023

optionsome Nov 23, 2023

t2gran Nov 21, 2023

t2gran Nov 21, 2023 •

edited

Loading

t2gran left a comment

t2gran Nov 21, 2023

optionsome Nov 23, 2023

optionsome commented Nov 23, 2023

leonardehrenfried left a comment

optionsome commented Nov 27, 2023

optionsome commented Dec 11, 2023

optionsome commented Dec 11, 2023

		? s0.getRequest().preferences().car().parking()
		: s0.getRequest().preferences().bike().parking();

Create own parking preferences for bike and car in the internal model #5521

Create own parking preferences for bike and car in the internal model #5521

Conversation

optionsome commented Nov 21, 2023 • edited Loading

Summary

Issue

Unit tests

Documentation

Changelog

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 Nov 21, 2023 • edited Loading

Codecov Report

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

t2gran Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

leonardehrenfried left a 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

t2gran Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

t2gran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

optionsome commented Nov 23, 2023

leonardehrenfried left a comment

Choose a reason for hiding this comment

optionsome commented Nov 27, 2023

optionsome commented Dec 11, 2023

optionsome commented Dec 11, 2023

optionsome commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

t2gran Nov 21, 2023 •

edited

Loading

t2gran Nov 21, 2023 •

edited

Loading