Skip to content
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

fix(core-utils): optimize flex combination generation #747

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daniel-heppner-ibigroup
Copy link
Contributor

This accomplishes two things. First, it only allows FLEX_DIRECT on its own, without transit or anything else. Although this should work, OTP has a bug that causes issues with this configuration. It doesn't make sense to combine them anyway.

Second, it makes FLEX (access/egress) only comaptible with TRANSIT. As far as I can tell, OTP doesn't support FLEX combined with bikeshare or anything else. This should reduce the number of queries we send to OTP, which will improve performance.

Lastly, it improves the test output to make it easier to read.

This accomplishes two things. First, it only allows FLEX_DIRECT on its own, without transit or anything else. Although this should work, OTP has a bug that causes issues with this configuration. It doesn't make sense to combine them anyway.

Second, it makes FLEX (access/egress) *only* comaptible with TRANSIT. As far as I can tell, OTP doesn't support FLEX combined with bikeshare or anything else. This should reduce the number of queries we send to OTP, which will improve performance.

Lastly, it improves the test output to make it easier to read.
@miles-grant-ibigroup
Copy link
Collaborator

A few things

  1. FLEX_DIRECT + BIKE is definitely a combination I've seen before. Flex and scooters as well, thats' why they're in the unit test!
  2. I agree flex access/egress should only be compatible with transit, but I don't think this approach is the way to do it. Can't we just reclassify these two in the same category as scooters/bikes?

I am hesitant to make these fundamental changes to the core of otp-ui when this is a performance problem that needs to be solved within OTP. Reducing the number of requests to the server isn't going to help performance, especially when we support load balancers

@daniel-heppner-ibigroup
Copy link
Contributor Author

Not sure about 1. Will have to investigate more.

For (2) I think this is a good approach, since it always felt like a hack to me to bundle flex with scooters and bike share. It really feels like its own thing. Especially when FLEX_DIRECT has to be treated separately.

As for performance, I sort of agree, but it is actually quite expensive to spin up more of these large instances that are used for flex-enabled OTP. We need to approach the performance problem from every angle, and especially this one if the extra requests aren't yielding any different results.

@daniel-heppner-ibigroup
Copy link
Contributor Author

daniel-heppner-ibigroup commented Jun 12, 2024

I think we've confirmed now that FLEX_DIRECT should also be alone. It doesn't make sense to add other modes since it's specifically door to door flex. For ACCESS and EGRESS, we might want to combine it with other access and egress modes.

The unit tests previously didn't differentiate between flex_direct and flex_access/egress, because the function itself didn't know the difference. Now it does, which is why I modified the unit tests as well.

@miles-grant-ibigroup
Copy link
Collaborator

hmm: ibi-group/OpenTripPlanner#215

@miles-grant-ibigroup miles-grant-ibigroup removed their assignment Jun 19, 2024
@daniel-heppner-ibigroup daniel-heppner-ibigroup removed their assignment Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants