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

Adding location_index field in output where the location field is present #625

Closed
krashish8 opened this issue Dec 12, 2021 · 3 comments
Closed

Comments

@krashish8
Copy link
Contributor

The location_index field is relevant whenever custom matrices are used. It plays a similar role as the location field when the routing engine is used. So, all the objects have both of these fields to be used depending on the situation. However, I see that in the output, only the location field is present (in the unassigned key, and the step object).

The user-defined matrices and the location_index type of fields were introduced in v1.2.0. Do we have any reasons for omitting the location_index type of field from the output, or is it because this field was not required then, so was not added?

I feel that it would be better to also add the location_index index field in the unassigned key, and the step object in the output. Firstly, it would look more consistent - both the location and location_index fields could be present in the output, depending on whether the custom matrices or the routing engine is used. Secondly, getting the location index of the task in the output might be useful to the users who are using custom matrices. Obviously, the end-users can get the location index of the task manually, but I think it would be easy to do it in the vroom itself without adding any extra complexity.

If this looks okay, I would be happy to submit a PR. :-)

@jcoupey
Copy link
Collaborator

jcoupey commented Dec 13, 2021

Technically, we provide a way to uniquely identify any task in output (step or unassigned) with type + id so users should be able to go back to their full description provided in input in the first place. In this sense, any additional information can be considered redundant. In order to avoid bloating the API, I've been trying to resist providing too much stuff in output "for convenience" and turned down such requests before, e.g. to feature initial constraints in output.

Now the first exception to this implicit rule is locations, which were included from the start for the sake of being able to simply plot a solution only from the output. I agree that not having location_index available in output while they are mandatory in input with custom matrices is not really consistent. So let's extend the location logic in output to location_index, of course only in the case where custom matrices are provided in input.

@jcoupey
Copy link
Collaborator

jcoupey commented Dec 13, 2021

@krashish8 if you submit a PR appending some stuff to unassigned objects, you may want to kill two birds with one stone by handling "description" too, see #403. ;-)

@krashish8
Copy link
Contributor Author

Actually, trying to kill "three" birds with one stone, where one bird is entirely different from the other two ;-)

Maybe it's not a good idea to kill different birds (issues) with a single stone (PR), but the changes were quite small, so I did. As a matter of fact, I'm getting rid of all the issues that I created here :-)

Please review: #627

@jcoupey jcoupey added this to the v1.12.0 milestone Dec 29, 2021
@jcoupey jcoupey closed this as completed in 3058d04 Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants