matrix pbf output #4121
matrix pbf output #4121
Conversation
…and replace MatrixType enum with protobuf definition
…ather than sources/targets
…ings in pbf and add the scaffolding to be able to return pbf
| } | ||
|
|
||
| MatrixType matrix_type = MatrixType::Cost; | ||
| Matrix::Algorithm matrix_algo = Matrix::CostMatrix; |
There was a problem hiding this comment.
I removed the MatrixType enum we kept around and replaced it with the PBF's one.
| /** | ||
| * Form a time/distance matrix from the results. | ||
| * @return Returns a time distance matrix among locations. | ||
| */ | ||
| std::vector<TimeDistance> FormTimeDistanceMatrix(); |
There was a problem hiding this comment.
it never had a function like this implemented (or well, at least after the recent PRs)
…ed Message, to enable int packing; solve the right order for forward/reverse tree
|
I took @SiarheiFedartsou 's suggestion and changed the proto definition to be "flat" vectors of the matrix attributes (times, dists etc), instead of a TimeDistance message "wrapper", so the int vectors can be packed. I also finished the forward vs reverse output order. |
| auto date_time = | ||
| get_date_time(origin_dt, origin_tz, pred_id, reader, static_cast<uint64_t>(time)); | ||
| out_date_times[pbf_idx] = date_time; |
There was a problem hiding this comment.
this pisses me off a bit.. we can't use the Resize() and Set() syntax for the string repeated fields, protobuf has no such concept for those.. I didn't really know how to do it any better, so:
- before we expand the sources or targets, we initialize an empty
std::vector<std::string>, which we pass into this function for every source or target forming - here we set the date_time string on the right index
- after we're all done with all expansions, we go over this vector and populate the PBF's
date_times
Any suggestion on how to make it less ugly is welcome.
| std::vector<std::vector<uint32_t>> matrix_answers = {{28, 28}, {2027, 1837}, {2403, 2213}, {4163, 3838}, | ||
| {1519, 1398}, {1808, 1638}, {2061, 1951}, {3944, 3639}, | ||
| {2311, 2111}, {701, 641}, {0, 0}, {2821, 2626}, | ||
| {5562, 5177}, {3952, 3707}, {4367, 4107}, {1825, 1680}}; |
There was a problem hiding this comment.
some human-healthier formatting :)
| TEST(Matrix, test_timedistancematrix_forward) { | ||
| // Input request is the same as `test_request`, but without the last target | ||
| const auto test_request_more_sources = R"({ | ||
| "sources":[ | ||
| {"lat":52.106337,"lon":5.101728}, | ||
| {"lat":52.111276,"lon":5.089717}, | ||
| {"lat":52.103105,"lon":5.081005} | ||
| ], | ||
| "targets":[ | ||
| {"lat":52.106126,"lon":5.101497}, | ||
| {"lat":52.100469,"lon":5.087099}, | ||
| {"lat":52.103105,"lon":5.081005}, | ||
| {"lat":52.094273,"lon":5.075254} | ||
| ], | ||
| "costing":"auto" | ||
| })"; |
There was a problem hiding this comment.
The bug in #4153 only occurred with the bicycle profile, the auto profile worked fine. Does that matter for your test/fix?
There was a problem hiding this comment.
Jep that's fine. Over the API we decide internally which matrix algo to use. In your case that was TDMatrix, for auto it's always CostMatrix. This test uses TDMatrix deliberately. Good point though!
| std::vector<std::vector<uint32_t>> expected_results = {{28, 28}, {2027, 1837}, {2403, 2213}, | ||
| {1519, 1398}, {1808, 1638}, {2061, 1951}, | ||
| {2311, 2111}, {701, 641}, {0, 0}, | ||
| {5562, 5177}, {3952, 3707}, {4367, 4107}}; |
There was a problem hiding this comment.
only changed formatting
| } | ||
| } | ||
|
|
||
| // TODO: it was commented before. Why? | ||
| TEST(Matrix, DISABLED_test_matrix_osrm) { | ||
| TEST(Matrix, test_matrix_osrm) { |
There was a problem hiding this comment.
to increase coverage, I test the OSRM matrix format properly. I guess the reason why it disabled was because it wasn't set up to actually test OSRM format, it just looked at the raw time/distances, not at the json output
fixes #3867
fixes #4153
Next up: isochrones.