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(matrix-algorithms): Consistent use of maximum visited nodes limit #1428

Merged
merged 10 commits into from
May 24, 2023

Conversation

aoles
Copy link
Member

@aoles aoles commented May 2, 2023

Apply the limit on the number of visited nodes which is specified in the config file to all matrix algorithms.

Pull Request Checklist

  • 1. I have rebased the latest version of the master branch into my feature branch and all conflicts
    have been resolved.
  • 2. I have added information about the change/addition to functionality to the CHANGELOG.md file under the
    [Unreleased] heading.
  • 3. I have documented my code using JDocs tags.
  • 4. I have removed unnecessary commented out code, imports and System.out.println statements.
  • 5. I have written JUnit tests for any new methods/classes and ensured that they pass.
  • 6. I have created API tests for any new functionality exposed to the API.
  • 7. If changes/additions are made to the ors-config.json file, I have added these to the ors config documentation
    along with a short description of what it is for, and documented this in the Pull Request (below).
  • 8. I have built graphs with my code of the Heidelberg.osm.gz file and run the api-tests with all test passing
  • 9. I have referenced the Issue Number in the Pull Request (if the changes were from an issue).
  • 10. For new features or changes involving building of graphs, I have tested on a larger dataset
    (at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
  • 11. For new features or changes involving the graphbuilding process (i.e. changing encoders, updating the
    importer etc.), I have generated longer distance routes for the affected profiles with different options
    (avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
    points generated from the current live ORS.
    If there are differences then the reasoning for these MUST be documented in the pull request.
  • 12. I have written in the Pull Request information about the changes made including their intended usage
    and why the change was needed.
  • 13. For changes touching the API documentation, I have tested that the API playground renders correctly.

Fixes #1246.

Information about the changes

  • Key functionality added: Enforce maximum_visited_nodes limit specified in the config file.

  • Reason for change: The limit was not respected at all by RPHAST matrix algorithm, and not handled properly by CoreMatrix either.

Examples and reasons for differences between live ORS routes, and those generated from this pull request

Some more complex long-distance matrix queries computed with either RPHAST or CoreMatrix which have been working before might start failing with the following error:

Unable to compute a distance/duration matrix: Search exceeds the limit of visited nodes.

This can be circumvented by increasing the maximum_visited_nodes limit if server resources allow it.

…rithms

Apply the same limit on the number of visited nodes which is specified in the config file to all matrix algorithms.
@takb takb added this to To do in ors general May 2, 2023
@MichaelsJP
Copy link
Member

MichaelsJP commented May 5, 2023

@aoles thanks for implementing this.

@MichaelsJP MichaelsJP self-requested a review May 5, 2023 12:22
Copy link
Member

@MichaelsJP MichaelsJP left a comment

Choose a reason for hiding this comment

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

I'd say we should have an integration/unit test in place checking, checking for this exception to be thrown. What do you think?

ors general automation moved this from To do to Review May 5, 2023
@aoles aoles self-assigned this May 10, 2023
@MichaelsJP MichaelsJP changed the title Consistent use of maximum visited nodes limit in matrix algorithms fix: Consistent use of maximum visited nodes limit in matrix algorithms May 13, 2023
…rithms

Apply the same limit on the number of visited nodes which is specified in the config file to all matrix algorithms.
@MichaelsJP MichaelsJP force-pushed the issue#1246-consistent_use_of_visited_nodes_limit branch from e1f8ec6 to 2ffe0ab Compare May 13, 2023 14:58
@github-actions github-actions bot added fix and removed fix labels May 13, 2023
@MichaelsJP MichaelsJP changed the title fix: Consistent use of maximum visited nodes limit in matrix algorithms fix(core-matrix): Consistent use of maximum visited nodes limit in matrix algorithms May 15, 2023
@github-actions github-actions bot added fix and removed fix labels May 15, 2023
@github-actions github-actions bot added fix and removed fix labels May 17, 2023
@MichaelsJP MichaelsJP force-pushed the issue#1246-consistent_use_of_visited_nodes_limit branch from 1ba6aac to e7bf917 Compare May 19, 2023 11:31
@MichaelsJP
Copy link
Member

@aoles Any progress so far or anything I could help with?

…rithms

Apply the same limit on the number of visited nodes which is specified in the config file to all matrix algorithms.
@MichaelsJP MichaelsJP force-pushed the issue#1246-consistent_use_of_visited_nodes_limit branch from e7bf917 to 73c2284 Compare May 23, 2023 08:48
@aoles aoles changed the title fix(core-matrix): Consistent use of maximum visited nodes limit in matrix algorithms fix(matrix-algorithms): Consistent use of maximum visited nodes limit May 23, 2023
@github-actions github-actions bot added fix and removed fix labels May 23, 2023
@aoles
Copy link
Member Author

aoles commented May 23, 2023

Thanks @MichaelsJP , I'm good 😎

Implementing reasonable unit tests turned out to be more effort than originally anticipated as the tests actually revealed some issues with the current implementation 🙈 Addressing these required some smart restructuring of existing methods and classes, that's why it took a bit longer. But I'm basically done now, just going through the introduced changes the last time before pushing to make sure nothing breaks 🤞

aoles added 2 commits May 23, 2023 11:54
Restructuring of the original implementation was neccassary in order to address some of the issues revealed by the unit tests and for the sake of reducing complexity and redundancy.
@aoles
Copy link
Member Author

aoles commented May 23, 2023

Unit test for exceeding the maximum number of visited nodes in RPHAST and Core matrix algorithms are now in. Regular Dijkstra matrix algorithm is currently not covered by any unit tests at all. I will look whether we could have at least an API test for it. I believe an additional integration test apart from the unit tests makes sense anyway.

aoles added 2 commits May 23, 2023 16:08
…rithms

Apply the same limit on the number of visited nodes which is specified in the config file to all matrix algorithms.
Restructuring of the original implementation was neccassary in order to address some of the issues revealed by the unit tests and for the sake of reducing complexity and redundancy.
@MichaelsJP MichaelsJP force-pushed the issue#1246-consistent_use_of_visited_nodes_limit branch from 48f01e9 to 1d8fbf2 Compare May 23, 2023 14:08
aoles added 2 commits May 24, 2023 15:58
Took the opportunity to refactor the test class a bit for the sake of DRYness.
@aoles
Copy link
Member Author

aoles commented May 24, 2023

@MichaelsJP With the API test in place, I think we are good to go now! 🚀

@MichaelsJP MichaelsJP enabled auto-merge (squash) May 24, 2023 16:27
@MichaelsJP MichaelsJP merged commit 827b667 into master May 24, 2023
16 checks passed
ors general automation moved this from Review to Awaiting release May 24, 2023
@MichaelsJP MichaelsJP deleted the issue#1246-consistent_use_of_visited_nodes_limit branch May 24, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
ors general
  
Awaiting release
Development

Successfully merging this pull request may close these issues.

RPHAST matrix algorithm does not respect config file setting of maximum_visited_nodes
2 participants