Skip to content

Add support to unprefer certain routes - #4238

Merged
hannesj merged 27 commits into
opentripplanner:dev-2.xfrom
joonaojapalo:unprefer-routes
Jul 19, 2022
Merged

Add support to unprefer certain routes#4238
hannesj merged 27 commits into
opentripplanner:dev-2.xfrom
joonaojapalo:unprefer-routes

Conversation

@joonaojapalo

@joonaojapalo joonaojapalo commented Jun 23, 2022

Copy link
Copy Markdown
Contributor

Summary

Add support to define penalty cost for unpreferred routes by request parameter or by config file. In router-config.json configuration file, routes are defined as list of feed scoped ids in routingDefaults.unpreferred.routes and associated cost routingDefaults.unpreferredRouteCost as linear function. For example:

{ 
  "routingDefaults": {
      "unpreferredRouteCost": "300 + 2.0 x",
      "unpreferred": {
        "routes": ["HSL:2146A", "HSL:31M1"]
      }
  }
}

Issue

Related issue: #4199

Unit tests

  • New test cased created for cost params building, for cost function and for a black-box routing case.
  • Solution has no observed performance implications (speed test and memory).

Changelog

The changelog file
is generated from the pull-request title, make sure the title describe the feature or issue fixed.
To exclude the PR from the changelog add [changelog skip] in the title.

@joonaojapalo
joonaojapalo requested a review from a team as a code owner June 23, 2022 17:27
@t2gran t2gran added this to the 2.2 milestone Jun 23, 2022
@leonardehrenfried

Copy link
Copy Markdown
Member

In case you're wondering why your build fails: things have changed in the dev-2.x branch and Github Actions always builds the merge of your branch and dev-2.x.

If you merge dev-2.x into your branch again you should see the same failure locally, which will make it easy to fix.

Comment thread src/main/java/org/opentripplanner/standalone/config/RoutingRequestMapper.java Outdated
transferConstraints
);

int routeReluctanceCost = routePenalties.getOrDefault(trip.routeId(), ZERO_COST);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, this is a total nit-pick but I want to avoid confusion: can you remove the word "reluctance" from the local variable name.

A reluctance is a cost that is proportional to the length of a leg. A penalty is applied only once.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point! Actually, at some point, the idea was to apply unfererence per transit leg length and variable naming is leftover.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To me the reluctance would actually be more useful, but I understand that you're just restoring the behaviour that was there before.

If I every have the need to unprefer routes I will implement the reluctance.

@joonaojapalo

Copy link
Copy Markdown
Contributor Author

As discussed in the developer meeting on 2022-06-28, I updated the fixed unpreference penalty to linear function (DoubleFunction<Double>). Speed test performance is not affected.

Comment thread src/main/java/org/opentripplanner/standalone/config/RoutingRequestMapper.java Outdated
Comment thread test/ci-performance-test/speed-test-config.json Outdated
T trip,
RaptorTransferConstraint transferConstraints
) {
return delegate.boardingCost(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right now this is not possible with the LinearFunction but I'm wondering if we should add the static cost here and then the coefficient in onTripRelativeRidingCost.

I believe that this would lead to the unpreferred routes being pruned earlier in the search and not when alighting the vehicle.

Am I seeing this correctly, @hannesj ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a good question! As @t2gran pointed out earlier in #4238 (comment), fixed penalty could be applied in boardingCost. Also as discussed earlier, reluctance might be the natural choice for cost policy in most scenarios and thus the optimization around the static cost might not be relevant.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We probably should extend, or create a new interface, which would support querying the fixed and variable parts of the function. This way we could split the calculation in two, and support multiple types of functions.

@codecov-commenter

codecov-commenter commented Jul 12, 2022

Copy link
Copy Markdown

Codecov Report

Merging #4238 (7aafd4c) into dev-2.x (868a1b2) will increase coverage by 0.11%.
The diff coverage is 74.32%.

@@              Coverage Diff              @@
##             dev-2.x    #4238      +/-   ##
=============================================
+ Coverage      56.87%   56.98%   +0.11%     
- Complexity     10555    10648      +93     
=============================================
  Files           1402     1415      +13     
  Lines          57118    57421     +303     
  Branches        6600     6624      +24     
=============================================
+ Hits           32485    32721     +236     
- Misses         22632    22699      +67     
  Partials        2001     2001              
Impacted Files Coverage Δ
...qlapi/datafetchers/LegacyGraphQLQueryTypeImpl.java 5.90% <0.00%> (-0.04%) ⬇️
...legacygraphqlapi/generated/LegacyGraphQLTypes.java 0.00% <0.00%> (ø)
...transit/frequency/FrequencyBoardOrAlightEvent.java 0.00% <0.00%> (ø)
...pplanner/transit/model/framework/FeedScopedId.java 53.12% <0.00%> (-1.72%) ⬇️
...dapter/transit/request/TripScheduleWithOffset.java 76.59% <50.00%> (-1.19%) ⬇️
...ripplanner/routing/api/request/RoutingRequest.java 72.92% <61.53%> (-2.14%) ⬇️
...rithm/raptoradapter/transit/cost/McCostParams.java 61.90% <85.71%> (+4.76%) ⬆️
...aptoradapter/transit/cost/RouteCostCalculator.java 93.33% <93.33%> (ø)
...toradapter/transit/cost/CostCalculatorFactory.java 90.00% <100.00%> (+6.66%) ⬆️
...aptoradapter/transit/cost/McCostParamsBuilder.java 100.00% <100.00%> (ø)
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 868a1b2...7aafd4c. Read the comment docs.

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At today's meeting we decided that we will postpone the decision on how to split the LinearFunction until @t2gran is back. @joonaojapalo has openend an issue for that.

The code here looks good so I will approve.

Comment thread docs/RouterConfiguration.md Outdated

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the documentation has a mistake in it. Can you confirm @joonaojapalo ?

@hannesj
hannesj requested a review from t2gran July 14, 2022 13:57
Co-authored-by: Thomas Gran <t2gran@gmail.com>
@hannesj
hannesj merged commit d2f79a0 into opentripplanner:dev-2.x Jul 19, 2022
t2gran pushed a commit that referenced this pull request Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants