Skip to content

Add bicycle triangle factors to Transmodel API#3585

Merged
leonardehrenfried merged 12 commits into
opentripplanner:dev-2.xfrom
leonardehrenfried:triangle-factors-for-transmodel-api
Sep 9, 2021
Merged

Add bicycle triangle factors to Transmodel API#3585
leonardehrenfried merged 12 commits into
opentripplanner:dev-2.xfrom
leonardehrenfried:triangle-factors-for-transmodel-api

Conversation

@leonardehrenfried
Copy link
Copy Markdown
Member

Summary

This exposes the bicycle triangle factors in the Transmodel API.

I also took care to return a meaningful error when the factors don't add up to 1. It looks like this:

{
  "data": {},
  "errors": [
    {
      "path": [
        "trip"
      ],
      "errorType": "DataFetchingException",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "message": "Exception while fetching data (/trip) : The values of triangleSafetyFactor, triangleSlopeFactor, and triangleTimeFactor must sum to 1"
    }
  ]
}

Unit tests

Is there a way to write a unit test for this? I'd love to.

Code style

Yes.

Documentation

Yes.

Changelog

Was a bullet point added to the changelog file with description and link to the linked issue?

@leonardehrenfried leonardehrenfried requested a review from a team as a code owner August 12, 2021 10:40
@gmellemstrand
Copy link
Copy Markdown
Contributor

I am not sure if others agree with me, but I think I would prefer the assertTriangleParameters method to just normalize the parameters so that they keep the same ratio, but the sum is 1. Just add a log statement whenever this happens instead of throwing an exception.

@leonardehrenfried
Copy link
Copy Markdown
Member Author

Should this only happen in the Transmodel API or everywhere?

@gmellemstrand
Copy link
Copy Markdown
Contributor

I think it would be good to do it everywhere, as I don't see that it is transmodel specific in any way.

@leonardehrenfried
Copy link
Copy Markdown
Member Author

I just saw that a method like that already exists but currently is totally unused:

public void setTriangleNormalized (double safe, double slope, double time) {

@gmellemstrand
Copy link
Copy Markdown
Contributor

Looks good! But could division by zero be a problem here?

@leonardehrenfried
Copy link
Copy Markdown
Member Author

I will write some tests to make sure.

Comment thread src/main/java/org/opentripplanner/routing/api/request/RoutingRequest.java Outdated
@leonardehrenfried leonardehrenfried force-pushed the triangle-factors-for-transmodel-api branch from 56779aa to 020e8fe Compare September 1, 2021 07:44
@t2gran t2gran added this to the 2.1 milestone Sep 2, 2021
@leonardehrenfried
Copy link
Copy Markdown
Member Author

leonardehrenfried commented Sep 2, 2021

@t2gran I've replaced the individual setters of the triangle factor with the call to the method where you have to set all three of them.

Comment thread src/ext/java/org/opentripplanner/ext/transmodelapi/TransmodelGraphQLPlanner.java Outdated
@leonardehrenfried leonardehrenfried force-pushed the triangle-factors-for-transmodel-api branch from 0d45f0f to 6df1ab2 Compare September 8, 2021 08:10
@t2gran t2gran added the !Improvement A functional improvement or micro feature label Sep 9, 2021
@leonardehrenfried leonardehrenfried merged commit ad64554 into opentripplanner:dev-2.x Sep 9, 2021
@leonardehrenfried leonardehrenfried deleted the triangle-factors-for-transmodel-api branch January 17, 2025 21:34
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.

4 participants