feature/custom_matrix#47

[PattyDePuh]

As suggested in #47 here is the PR of the branch, that is still in progress:

• Custom-Matrix import reamins untouched - no push_back's anymore.
• start_id is now optional and the sanity check, if either start_id end_id available, is inside add_vehicle

What is was ToDo:

• usage of location::index as general id for location in matrix (At least thats, what i understood)
• writing guideline for usage of the new official feature

Editing the code style: I would prefer to do that after the pull in an own branch. I do like the spaces between parantheses, brackets and control structures, but that wasn't consistent so far.

[jcoupey]

Thanks for the WIP PR.

Trying to expand on what I had in mind regarding dealing with matrix indices. Job and vehicle ids are just a way of identification between input and output data, they might be any number (think job ids exported from a database) and are uncorrelated with actual index of any location in the matrix.

So any place like jobs or vehicle start/end embed a location as defined in src/structures/vroom/location.h. I've already made coordinates optional in this struct with the custom matrix feature in mind. The index member of a location is what is used throughout the code base whenever we need to fetch a cost in the matrix. So two options here:

• if a user-matrix is provided, all jobs should have an index field and vehicles should have start_index and/or end_index fields, and the locations index should be set accordingly upon location construction by add_job and add_vehicle functions
• if no user-matrix is provided (current implementation), then in the input class, locations are assigned incremental numbers for their index using current _location_number. Locations are stored in the same order in _ordered_locations which is used for matrix computing to ensure consistency in the matrix and locations indices.

Hope that helps!

[PattyDePuh]

location::index is now properly used, as desired.

[PattyDePuh]

Is there a deeper reason behind the assert at
https://github.com/PattyDePuh/vroom/blob/json-matrix%2347/src/problems/tsp/tsp.cpp#L54
because in the current situation it prevents to compute a roundtrip, where start_id and end_id are the same.
I could wirte a work-around by adding the start location twice, and create a expanded Matrix with get_sub_matrix accordingly - with in mind, that this will maybe raise complexity for issue #57 .

Maybe there are no further side effects, if the assert is deleted and _matrix[_end][_start] = 0; shifted behind the loop. Then there is no workaround necessary. @jcoupey

[jcoupey]

About the assert, there would be other bad side effects during solving. The main problem is having a 0 on the diagonal, hence this. Currently, similar start and end locations are duplicated in the matrix, which is really a work-around we should avoid with the custom matrix use-case.

I should probably try to fix the problem raised by having a 0 on the diagonal. This requires a deep dive into the munkres implementation I had hoped would stay happily untouched for long... This would make sense in the light of #57 and would also allow to avoid location duplicates in any case.

I'd suggest keeping the assertion so far and consider the round-trip case with same start/end as not handled (yet) in the PR.

[jcoupey]

@PattyDePuh just added a commit to your branch to avoid having things showing up in diffs (or github compare) only because of formatting differences.

[jcoupey]

Regarding the jobs definition, I think the jobs json array should remain mandatory rather than trying to automatically guess jobs from the lines of the matrix that are different from start and end index. I see several reasons for this:

• API consistency (except for the *_index keys)
• keeping id and index as two completely different things (users should be able to provide any job id that is convenient for them)
• inferring jobs like this won't be enough anymore as soon as we allow adding other constraints to the jobs

I'm fine with the input description you suggested before with location_index as keyword.

[PattyDePuh]

ok, for an consistent output, i adjusted the step class:
instead of an job attribute, it stores an general identifier id plus an optional location_indexwith a flag, that location_index is initialized. Depending on this flag, the json_output prints the location_index into the step.
An identifier for steps of type start and end is also provided from now on, but will not be printed out.

[PattyDePuh]

This part is still bugging me, since i don't have a flag inside the output_json-class to indicate, that this should only happen, if a custom matrix is provided.

[PattyDePuh]

With the latest commit we should be good to go now, i guess.
So we can continue with #57
then we will take a look at munkres matrix manipulation
and finally solve the duplicate problem.

[jcoupey]

Thanks @PattyDePuh for the ongoing work! I'm sorry I don't have much time to review this properly right now. I'll try to find some time soon to take a deeper look... Just two quick comments for now:

• you mention being bugged by the flag that indicates a custom matrix is used. I think the input class should be totally json unaware in order to be usable from C++ only (hence input_parser doing all the work in-between). So maybe we should find another place where this flag belongs.

• I think you don't need to add anything to the step class as the location passed to the constructor already have an index member.

[PattyDePuh]

About the boolean flag - that was only for the location identifier during the output, since i wanted to achieve, that both the id and the location_index be printed out from the start / end / job without influencing the preexisted output. But on third thought, the first identifier alone identifies the job points and the start/end points are identifyable by the vehicle-rank.
So i will remove my step and output changes...

About the input-class, that should be unaware of the json-matrix:
I could try to achieve that, also ... need to look for replacements inside input.cpp and tsp.cpp.
Will update my results here soon.

[PattyDePuh]

/done
Several classes (including tsp.cpp) are now untouched.
The whole import is handeled inside input_parser and inside the overloaded functions of input.
input class has no bool-flag anymore, that indicates a custom matrix - staying totally unaware of it.
The only moment, when it does influence its behavior is during set_matrix(), where it checks, if the matrix got some entries or no - influencing, if the osrm-wrapper is used.

[PattyDePuh]

@jcoupey is the code review still in process?

[jcoupey]

@PattyDePuh I just want to find enough time to go through the changes in detail, which I wasn't able to do since our last exchanges.

But I'm positive about this getting merged soon, so meanwhile I'm sorry for the delay here, especially as you've been promptly reacting to all suggestions!

[jcoupey]

Hey @PattyDePuh I went through the code and just did a few adjustments. I checked that the behaviour didn't change while solving with osrm.

Using the matrix key in json input seems to work provided every matrix index is used exactly once in a job or vehicle location. This is something I didn't anticipate, due to the fact that the TSP solving code is really based on the matrix it is fed, in a way that is quite unrelated from the jobs description in the API.
If you remove one job from a working example (say the one you put in API.md), then the solution based on the matrix still has the same number of visited places and when trying to get back to the jobs to format the resulting route, we hit an assertion about some job not found for the extra index.

In a way it could make sense to require the matrix to be exactly the "right" one for the problem but from an API point of view, it feels odd. For example, one might want to modify an input problem by simply removing a job without bothering to remove the matching line and column in the matrix. In such a case I guess one would expect the unnecessary line/column to just be ignored during solving.

[PattyDePuh]

You got there a point - the current implementation is strict with the consideration, that the input json is computer generated and no human dares to alter the in existing input files.
Will take a look, how to adjust, that the matrix does not have to be fully used, if jobs are sparse ...

[jcoupey]

I think we can store the full matrix in input and have the TSP code somehow operate on a submatrix based on the relevant indexes. This probably requires other changes to the interfaces and input members, but it's definitely mandatory to be able to do this in the long run (for example it could be used when clustering the whole problem based on the number of vehicles in order to solve several TSP instances).

I hope to be able to give it a go in the next few days!

[PattyDePuh]

The solution, that i pushed right now, filters the matrix from the unused indexes before storing it in the input class - making use of the get_sub_matrix method.
The solution was made before i saw your comment, but i think it goes in the same direction. ^^

That shall solve the inconvinient need to adjust the matrix in the inputfile, if somebody messes with the jobs.
The argument from you tends to overengineer, i guess. Let's implement the storing function, when we really need it.

[jcoupey]

@PattyDePuh the filtering you implemented works just fine, yet I see two drawbacks:

• User-defined indexes are not retained (they are renumbered), which could be a limitation e.g. if at some point we want to had them to the response.
• This works only for json input. It means that if we want to be able to set input entirely in C++ through add_job and add_vehicle, we'll hit the same problem if a user removes one add_job without adjusting the whole input matrix.

I want to retain your filtering logic but move it after input description and before the creation of the problem instance. Working on this right now!

EDIT: will ticket this and address after merging.