Skip to content

Dynamically compute additional search days - #3824

Merged
leonardehrenfried merged 16 commits into
opentripplanner:dev-2.xfrom
ibi-group:make-search-days-configurable
Feb 3, 2022
Merged

Dynamically compute additional search days#3824
leonardehrenfried merged 16 commits into
opentripplanner:dev-2.xfrom
ibi-group:make-search-days-configurable

Conversation

@leonardehrenfried

Copy link
Copy Markdown
Member

Summary

In order to boost performance make it possible to configure the additional search days to consider when filtering the trips.

Issue

n/a

Unit tests

none

Code style

Yes.

Documentation

Javadoc updated.

@leonardehrenfried
leonardehrenfried requested a review from a team as a code owner January 20, 2022 16:13
hannesj
hannesj previously approved these changes Jan 20, 2022
@t2gran t2gran added this to the 2.1 milestone Jan 24, 2022
@t2gran t2gran added the !Improvement A functional improvement or micro feature label Jan 24, 2022

@t2gran t2gran 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 am a bit unsure, but I think the additionalDays(Before|After)SearchTime can be simplified and improved - I think we forgot to question this when the new pattern index on running date was made.

Comment thread src/main/java/org/opentripplanner/routing/api/request/RoutingRequest.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/api/request/RoutingRequest.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/api/request/RoutingRequest.java Outdated
@leonardehrenfried

Copy link
Copy Markdown
Member Author

@hannesj @t2gran I've reworked the calculation according to your spec. However, I set the static maximum trip time because I can't remember how we wanted to pass that to the transit router. How did we say we wanted to implement that?

@leonardehrenfried leonardehrenfried changed the title Make additional search days configurable, change default to 1 Dynamically compute additional search days Jan 25, 2022
@hannesj

hannesj commented Jan 25, 2022

Copy link
Copy Markdown
Contributor

@hannesj @t2gran I've reworked the calculation according to your spec. However, I set the static maximum trip time because I can't remember how we wanted to pass that to the transit router. How did we say we wanted to implement that?

I think we could have a parameter with max hours of travel duration, which could be loaded from the config, in a similar manner you originally proposed.

Comment thread src/main/java/org/opentripplanner/routing/api/request/RoutingRequest.java Outdated

@t2gran t2gran 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.

Sorry for being a bit hash and direct in my review here 🤒, but I just trying to explain what I think is important from a design perspective. I think we need to break thing apart, before we can assemble them into better encapsulated components later.

Comment thread src/main/java/org/opentripplanner/standalone/config/RouterConfig.java Outdated
Comment thread src/test/java/org/opentripplanner/standalone/config/RouterConfigTest.java Outdated
@leonardehrenfried

Copy link
Copy Markdown
Member Author

Sorry for being a bit hash and direct in my review here face_with_thermometer, but I just trying to explain what I think is important from a design perspective. I think we need to break thing apart, before we can assemble them into better encapsulated components later.

That's quite ok - it's what reviews are there for.

How about we talk about how to resolve the deeper architectural problems (time zones) tomorrow's meeting?

@leonardehrenfried
leonardehrenfried force-pushed the make-search-days-configurable branch 2 times, most recently from ac15e5e to 66aa175 Compare January 31, 2022 12:46
@hannesj

hannesj commented Feb 1, 2022

Copy link
Copy Markdown
Contributor

I think we can merge this without the ability to configure the maxJourneyDuration, and instead do that after #3828 is merged.

@leonardehrenfried
leonardehrenfried force-pushed the make-search-days-configurable branch from f931a31 to c98075a Compare February 2, 2022 13:02
@leonardehrenfried
leonardehrenfried force-pushed the make-search-days-configurable branch from 3af54e6 to 9349593 Compare February 2, 2022 13:59
@leonardehrenfried
leonardehrenfried merged commit eb487b8 into opentripplanner:dev-2.x Feb 3, 2022
@leonardehrenfried
leonardehrenfried deleted the make-search-days-configurable branch February 3, 2022 12:53
t2gran pushed a commit that referenced this pull request Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants