Make edge metrics strongly typed #6421
Conversation
| return (word & ~mask) | ((static_cast<WordT>(value) >> offset) & mask); | ||
| } | ||
|
|
||
| template <typename WordT, typename T> |
There was a problem hiding this comment.
These are needed to allow PackedVectors of Alias types to be correctly serialized.
| } | ||
|
|
||
| // FIXME: This is needed for tests on Boost ranges to correctly compare Alias values. | ||
| template <typename F, typename U> bool operator!=(const osrm::Alias<F, U> value) const |
There was a problem hiding this comment.
This is a bit of a hack. The unit tests compare PackedVector ranges of Alias types against normal vectors of Alias types. Boost seems to know what to do for integral types, but this was the only way to get the comparison to work. Open to suggestion of how to do this in a better way.
| TurnPenalty{std::numeric_limits<TurnPenalty::value_type>::max()}; | ||
| static const EdgeDistance INVALID_FALLBACK_SPEED = | ||
| EdgeDistance{std::numeric_limits<EdgeDistance::value_type>::max()}; | ||
| // TODO: These are the same as the invalid values. Do we need both? |
There was a problem hiding this comment.
Potentially worth revisiting. We have invalid and maximal constants for the same numeric max. Could be consolidated.
| return getPathDistance(facade, unpacked_path, source_phantom, target_phantom); | ||
| } | ||
|
|
||
| std::tuple<EdgeWeight, EdgeDistance> getLoopWeight(const DataFacade<Algorithm> &facade, NodeID node) |
There was a problem hiding this comment.
getLoopWeight had to be split into two functions, as it's now returning different types depending on the input.
There was a problem hiding this comment.
I bet we can unify them using some metaprogramming magic, but up to you
There was a problem hiding this comment.
I think I've figured out how to do it.
| static_cast<const typename FromAlias::value_type>(from))}; | ||
| } | ||
|
|
||
| template <typename ToNumeric, typename FromAlias> inline ToNumeric from_alias(const FromAlias &from) |
There was a problem hiding this comment.
Open to better naming suggestions for these functions.
|
#3601 (comment) seems to imply that there will be no performance hit, but I'll do some testing to confirm. |
mjjbell
force-pushed
the
mbell/fix_compress_penalty
branch
from
b81044a to
29a1bbd
Compare
|
LGTM |
This change takes the existing typedefs for weight, duration and distance, and makes them proper types, using the existing Alias functionality. Primarily this is to prevent bugs where the metrics are switched, but it also adds additional documentation. For example, it now makes it clear (despite the naming of variables) that most of the trip algorithm is running on the duration metric. I've not made any changes to the casts performed between metrics and numeric types, they now just more explicit.
mjjbell
force-pushed
the
mbell/fix_compress_penalty
branch
from
29a1bbd to
19fef70
Compare
|
Timed the processing steps using the great-britain extract and the node tests on my 12 core dev machine. Not particularly scientific, but I think it's sufficient to show it's had no adverse effects on performance.
|
This change takes the existing typedefs for weight, duration and distance, and makes them proper types, using the existing Alias functionality. Primarily this is to prevent bugs where the metrics are switched, but it also adds additional documentation. For example, it now makes it clear (despite the naming of variables) that most of the trip algorithm is running on the duration metric. I've not made any changes to the casts performed between metrics and numeric types, they now just more explicit.
Issue
This change takes the existing typedefs for weight, duration and distance, and makes them proper types, using the existing
Aliasfunctionality.Primarily this is to prevent bugs where the metrics are switched, but it also adds additional documentation. For example, it now makes it clear (despite the naming of variables) that most of the trip algorithm is running on the duration metric.
I've not made any changes to the casts performed between metrics and numeric types, they are now just more explicit.
Tasklist
Requirements / Relations
Closes #3601