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

Aligning MultiPointTripPolicyGroup with TripPolicyGroup #333

Merged
merged 7 commits into from
Mar 13, 2023

Conversation

trurlurl
Copy link
Contributor

@trurlurl trurlurl commented Mar 9, 2023

Shouldn't the recent additions to the TripParams also be available to MultiPointTripRequest?

OptimisationMethods, ItModesToCover, ConsiderElevationData

@trurlurl trurlurl added bug Something isn't working documentation labels Mar 9, 2023
@trurlurl trurlurl added this to the v2.0 milestone Mar 9, 2023
@trurlurl trurlurl requested a review from ue71603 March 9, 2023 12:10
ue71603
ue71603 previously approved these changes Mar 9, 2023
skinkie
skinkie previously approved these changes Mar 9, 2023
Copy link
Contributor

@sgrossberndt sgrossberndt left a comment

Choose a reason for hiding this comment

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

There is a BaseTripPolicyGroup which is used by MultiPointTripPolicyGroup and TripPolicyGroup only. In my opinion all elements belonging to both MultiPointTripPolicyGroup and TripPolicyGroup should be put into BaseTripPolicyGroup. With the current state of this change the OptimisationMethod-choice would be aligned but at least ConsiderElevationData makes sense in MultiPointTripPolicyGroup as well, right? With the current definition of ItModesToCover in my opinion it does not make sense to add it to MultiPointTripPolicyGroup as well, which would mean all elements are part of BaseTripPolicyGroup, only TripPolicyGroup has the additional ItModesToCover and only MultiPointTripPolicyGroup has the additional MultiPointType.
ConsiderElevationData must be moved in front of ItModesToCover in order to have it in BaseTripPolicyGroup, but it was only recently added, so its no big deal, right?

@ue71603
Copy link
Contributor

ue71603 commented Mar 10, 2023

@trurlurl Can you do all the changes according to Stephan or should I do it? And yes: They should be harmonized fully.

@sgrossberndt
Copy link
Contributor

Working on it.

… to BaseTripPolicyGroup

ItModesToCover is left in each PolicyGroup as it has a different meaning (and thus documentation) for MultiPointTripPolicyGroup and TripPolicyGroup
@sgrossberndt sgrossberndt dismissed stale reviews from skinkie and ue71603 via 7804766 March 10, 2023 08:53
sgrossberndt
sgrossberndt previously approved these changes Mar 10, 2023
Copy link
Contributor

@sgrossberndt sgrossberndt left a comment

Choose a reason for hiding this comment

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

will update again

@sgrossberndt
Copy link
Contributor

ItModesToCover has the definition:

For each mode in this list a separate monomodal trip shall be found - in addition to inter-modal solutions.

Does this make sense for a MultiPointTripPolicyGroup for you? I'd rather not add it there, I think it is on purpose this was only part of TripPolicyGroup and not MultiPointTripPolicyGroup.

@trurlurl
Copy link
Contributor Author

ItModesToCover has the definition:

For each mode in this list a separate monomodal trip shall be found - in addition to inter-modal solutions.

Does this make sense for a MultiPointTripPolicyGroup for you? I'd rather not add it there, I think it is on purpose this was only part of TripPolicyGroup and not MultiPointTripPolicyGroup.

I could imagine that it could make sense when distributing a search for monomodal trips by calling OJPMultiPointTripRequest for parts of the trip - and that these parts should also be monomodal.

@ue71603
Copy link
Contributor

ue71603 commented Mar 10, 2023

@sgrossberndt I think you are right about ItModesToCover. That shouldn't be done in MPTR

@ue71603
Copy link
Contributor

ue71603 commented Mar 10, 2023

I think we can define MPTR that it is used in the middle of distributed trip planning and I think if one ones monomdal trips through Europe, one does this directly on one iv trip planner.

@skinkie skinkie merged commit 7691e00 into changes_for_v1.1 Mar 13, 2023
@skinkie skinkie deleted the aligning_tripparampolicies branch March 14, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working doc updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants