feat: add support for maps with traffic#2320
feat: add support for maps with traffic#2320diogodanielsoaresferreira wants to merge 10 commits into
Conversation
|
To revive the open point from https://github.com/TimefoldAI/timefold-models-sdk/pull/975, let me copy here this summary:
I see a problem with the second row. We should still return N matrices with N timeframes. Opposed to row number 3, the matrices will be larger, as we cannot prune them by the location availabilities. |
|
Ad "for now, getWaypoints still use a map with timeframe "static", which means that for models to use it, we always need the 3 timeframe maps + static one". Does it mean we use different routes for computing the travel time (assuming the traffic data leads to different routes) than we show in the waypoints? Discussed separately:
|
Why do we want to do this? my idea was that the model interface would be a clear separation between using/not using timestamps: if using LocationsAndTrafficAware, a departure time would always have to be sent to getTravelTime; if not, a departure time could not be sent. If we want to allow to have LocationsAware with multiple matrices, then we will have to change that assumption. So the new table would be something like this:
@rsynek is this your idea? |
|
Ad Ad Ad "clear separation between using/not using timestamps" : Ad "clients can always fetch a travelTime using or not using departureTime: this will not fail, but it will accept everything silently and redirect to the correct matrix" : if traffic data is enabled, we could write a warning when General requirements
|
|
@rsynek Those requirements should be covered by the last commit, so:
|
rsynek
left a comment
There was a problem hiding this comment.
Thanks for the adjustments!
I am sharing some comments inline; please have also look at the SonarCloud issues. Not saying we have to always change the code according to SonarCloud, but let's consider them and if it's the expected behavior, I can mark the issues as false positives.
I think primarily the ones about equals() and hashcode() in records with arrays are pulling the overall mark down.
| String location = config.getOptionalValue(MODEL_CONFIG_MAP_LOCATION, String.class).orElse(null); | ||
| Double maxDistanceFromRoad = config.getOptionalValue(MODEL_CONFIG_MAP_DISTANCE, Double.class).orElse(null); | ||
| String transportType = config.getOptionalValue(MODEL_CONFIG_MAP_TRANSPORT_TYPE, String.class).orElse(null); | ||
| Boolean useTraffic = config.getOptionalValue(MODEL_CONFIG_MAP_USE_TRAFFIC, Boolean.class).orElse(null); |
There was a problem hiding this comment.
Since it's Boolean, should the default value be false?
There was a problem hiding this comment.
We can change to false for completeness sake. Done
| * @return the locations relevant to the plan, mapped to the time intervals during which each location may be | ||
| * involved in travel. Each {@link TimeInterval} covers a continuous range {@code [from, to]}; the enricher | ||
| * determines which timeframe buckets the interval overlaps and fetches only the matrices needed for those | ||
| * buckets. A location relevant to only one timeframe needs a single narrow interval; a location active | ||
| * across the whole day would have a wider interval (or multiple intervals). Must not be {@code null} or | ||
| * empty during enrichment. Every key must also appear in {@link #getLocations()}. |
There was a problem hiding this comment.
Nitpick: usually, return is typically a one liner, summarizing what the method returns. Longer texts explaining the logic in details better fir the general description of the method.
There was a problem hiding this comment.
I agree, moved most text to the description of the method.
| List<Location> locationsNotInMap) { | ||
| } | ||
|
|
||
| private DistanceMatrix zeroMatrixFor(List<Location> locations) { |
There was a problem hiding this comment.
I thinks this SonarCloud check is relevant: if the method is static, just be looking at the declaration, the reader knows it's a pure function and does not touch the instance fields. Useful for debugging, especially in case multi-threaded computing.
There was a problem hiding this comment.
Updated to be a static method.
| return new AssembledTimeframedMatrices(travelTimesByTimeframe, distancesByTimeframe, locationsNotInMap); | ||
| } | ||
|
|
||
| private record AssembledTimeframedMatrices(DistanceMatrix[] travelTimesByTimeframe, |
There was a problem hiding this comment.
This Sonar issue puts down the mark to C, so better to resolve it.
The way records handle arrays in equals(), hashCode() or toString(), is using purely the reference (and not the content).
What is the expected equals() and hashCode() contract for this class?
There was a problem hiding this comment.
Added those methods. This class is only used as a wrapper for communication between the client and the enricher, but it doesn't hurt to add those.
| CompletableFuture<TravelTimeAndDistanceWithMetadata>[] futures = new CompletableFuture[n]; | ||
| List<Location>[] subsets = new List[n]; | ||
|
|
||
| for (int idx = 0; idx < n; idx++) { |
There was a problem hiding this comment.
n does not tell much. Caching the size as in int n = allTimeframes.size(); is a premature optimization that reduces readability.
Collections implement size() in an efficient way that does not require caching:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/ArrayList.java#L253
There was a problem hiding this comment.
Changed the name to allTimeframesSize.
| CompletableFuture<?>[] pending = Arrays.stream(futures).filter(Objects::nonNull) | ||
| .toArray(CompletableFuture<?>[]::new); | ||
| if (pending.length > 0) { | ||
| CompletableFuture.allOf(pending).join(); |
There was a problem hiding this comment.
I suppose the code blocks here until all threads finish, is that correct?
There was a problem hiding this comment.
Yes, the code blocks until all threads finish. We don't have any upper bound time limit for single calls, since they can take a long time, so I opted to also not add it here.
| * {@code locationsNotInMap} is the union of locations that fell out of the map across every timeframe call; the list | ||
| * preserves insertion order of the caller's original location list and contains no duplicates. | ||
| */ | ||
| public record TravelTimesByAvailabilityWithMetadata(DistanceMatrix[] travelTimesByTimeframe, |
There was a problem hiding this comment.
The same concern as in the other record; the equals and hashcode contract for arrays.
There was a problem hiding this comment.
Added equals, hashcode and toString.
| * ({@link #setTravelTimeMatrices(DistanceMatrix[], ToIntFunction)}) or with a single matrix | ||
| * ({@link #setTravelTimeMatrix(DistanceMatrix)}): the per-timeframe matrix for {@code departureTime} is used when | ||
| * timeframe matrices are configured, otherwise the single matrix is used and {@code departureTime} is ignored. |
There was a problem hiding this comment.
Nitpick: for API classes, I would talk in general terms (traffic data was requested or not) and avoid implementation details.
There was a problem hiding this comment.
Makes sense, removed the implementation details.
| */ | ||
| public TravelDistance getDistanceTo(Location location) { | ||
| if (distanceMatrix == null) { | ||
| DistanceMatrix matrix = distanceMatrix != null ? distanceMatrix : firstAvailableMatrix(distancesByTimeframe); |
There was a problem hiding this comment.
I suppose the firstAvailableMatrix is there for the situation when traffic data was enabled, but we don't ask for it, correct?
Instead of finding the first available matrix on the hot path, why don't we select the right one and set it as the distanceMatrix reference during enrichment?
There was a problem hiding this comment.
Yes, that's correct. We don't do that because we don't know which is the "right" one, that's why we just take the first. We can do this in the enricher and set the first matrix as the "single" matrix, although it's a tradeoff: we get the optimizations for single matrix, but the object will take more memory (3 matrices + 1 repeated matrix for requests without timestamp).
|


Summary
Adds support for traffic-aware travel times in the maps model. Travel-time matrices can now be enriched with time-of-day traffic information using configurable time-frame bucketing strategies, enabling solver models to plan with realistic, traffic-sensitive routing.
Changes
New opt-in enrichment path for solver models whose travel times and distances vary with time of day. Existing LocationsAwareSolverModel + TravelTimeMatrixEnricher untouched.