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

Report NO_TRANSIT_CONNECTION when search-window is set. #5570

Merged
merged 2 commits into from Dec 19, 2023

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Dec 13, 2023

Summary

The computation of NO_TRANSIT_CONNECTION is based on the the existence of the searchWindowUsed. This is not correct, and this PR make encapsulate this logic and uses the path and heuristic results in raptor instead.

Issue

🟥

Unit tests

🟥 The code here is not great and it is difficult to write a proper test. I do not have time to clean it up or write test for this, but I think it is better to fix the bug - then noting.

Documentation

✅ I have updated the doc in a few places.

Changelog

✅ This bug have probably existed in OTP for a long time.

Bumping the serialization version id

🟥 No needed.

@t2gran t2gran added the bug label Dec 13, 2023
@t2gran t2gran added this to the 2.5 (next release) milestone Dec 13, 2023
@t2gran t2gran requested a review from a team as a code owner December 13, 2023 12:28
@t2gran t2gran force-pushed the otp2_fix_no_transit_connection branch from 587bb53 to 75d2f1f Compare December 13, 2023 12:34
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

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

Comparison is base (4230fd3) 67.35% compared to head (bc84e9a) 67.24%.
Report is 1 commits behind head on dev-2.x.

❗ Current head bc84e9a differs from pull request most recent head 51deb2b. Consider uploading reports for the commit 51deb2b to get more accurate results

Files Patch % Lines
...ripplanner/raptor/api/response/RaptorResponse.java 50.00% 0 Missing and 1 partial ⚠️
.../algorithm/raptoradapter/router/TransitRouter.java 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5570      +/-   ##
=============================================
- Coverage      67.35%   67.24%   -0.12%     
+ Complexity     16162    16085      -77     
=============================================
  Files           1858     1852       -6     
  Lines          71093    70968     -125     
  Branches        7403     7399       -4     
=============================================
- Hits           47888    47723     -165     
- Misses         20745    20777      +32     
- Partials        2460     2468       +8     

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

Comment on lines +66 to +68
* Return {@code true} if the heuristic and the main search does not find any connections.
* Searching again with another time/search-window will not produce any results. There is no paths
* in the set of days provided in the transit data with the request usd.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I don't get it but doesn't this javadoc contradict the documentation of the error type?

Copy link
Member Author

Choose a reason for hiding this comment

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

The doc on noTransitConnectionInSearchWindow is changed in this PR, but this is mapped to noTransitConnection.

noTransitConnection - "No transit connection was found between the origin and destination withing the operating day or the next day"

noTransitConnectionInSearchWindow- "Transit connection was found, but it was outside the search window, see metadata for the next search window"

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, we return noTransitConnection when you should not page and noTransitConnectionInSearchWindow when you should.

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

I have a question about the docs: does it or doesn't it help to search with another page/search window?

@t2gran t2gran merged commit f16c057 into opentripplanner:dev-2.x Dec 19, 2023
5 checks passed
@t2gran t2gran deleted the otp2_fix_no_transit_connection branch December 19, 2023 19:40
t2gran pushed a commit that referenced this pull request Dec 19, 2023
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.

None yet

3 participants