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

Return datasource names along with datasource annotation #4973

Merged
merged 4 commits into from
Apr 3, 2018

Conversation

danpat
Copy link
Member

@danpat danpat commented Mar 23, 2018

Issue

We currently expose the datasources annotation, which returns the index of the datasource as supplied with the --segment-speed-file parameter to osrm-contract or osrm-customize.

This PR exposes the string version of that - the actual filename (minus file extension) that was passed to --segment-speed-file. This is useful for getting a human readable version of the annotation, rather than needing to jump through hoops to figure out which index matches to which file supplied.

The new format adds a metadata sub-structure, and the datasource_names list resides therein:

annotations: {
    datasources: [ 0, 1, 2, 3 ],
    metadata: {
        datasource_names: [ 'lua profile', 'traffic1', 'traffic2', traffic3' ]
    }
}

Tasklist

@danpat danpat added this to the 5.17.0 milestone Mar 23, 2018
@danpat danpat force-pushed the datasource_name_annotations branch from c1c6e66 to 52eb986 Compare March 23, 2018 21:43
@danpat danpat requested a review from TheMarex March 23, 2018 21:44
Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a stab at this! But we should re-consider adding a new annotation type for this. It might be easier to just add a new property to the RouteLeg object something like datasource_name that lists the names of the data sources if you pass annotations=datasources. That would get around the old "many strings in vector" problem and be more consistent with the current API. Basically we only need to add the dictionary for our dictionary encoded data sources.

@danpat
Copy link
Member Author

danpat commented Mar 26, 2018

Ah, good idea. How about just putting it inside the annotations object, like so?

  routes: [ {
     annotations: { datasources: [0,2,1,1,1,2,1],
                    datasource_names: ["lua profile","traffic1","traffic2"]
  }
  }]

@TheMarex
Copy link
Member

Ah, good idea. How about just putting it inside the annotations object, like so?

Yeah that could work, the only concern is that people could be confused about the fact that it behaves differently then other entries in annotations.

@danpat
Copy link
Member Author

danpat commented Mar 26, 2018

@TheMarex I know, I wish we'd thought ahead and could put this under datasources: { names: [...], ids: [...] } or something.

I'll put it under annotations, at least that way it's somewhat tied to the annotations= query parameter. I'll make sure the docs are up-to-date. Although most annotations work on a per-segment or per-coordinate basis, there's no contract that says they have to.

An alternative, maybe to try to future-proof this a bit, might be to add a metadata thing inside annotations, like:

{
  annotations: {
    metadata: { datasource_names: [...] },
    datasources: [....],
    speeds: [...]
  },
  ...
}

This would give us a spot to put future metadata like this so we could say "the metadata property contains data used by other annotations" or something, and would be the only exception to the per-segment/per-node pattern.

@danpat danpat force-pushed the datasource_name_annotations branch from 52eb986 to c9dfad4 Compare March 27, 2018 21:17
@danpat danpat self-assigned this Apr 2, 2018
@danpat danpat force-pushed the datasource_name_annotations branch from c9dfad4 to 068e80a Compare April 3, 2018 00:53
@danpat danpat changed the title Add new datasource_names annotation Return datasource names along with datasource annotation Apr 3, 2018
@danpat
Copy link
Member Author

danpat commented Apr 3, 2018

@TheMarex I moved everything under a new metadata sub-structure. Want to take another look?

@TheMarex
Copy link
Member

TheMarex commented Apr 3, 2018

@danpat one of the unit tests is failing:

/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/route.cpp(456): error in "test_manual_setting_of_annotations_property": check annotations.size() == 5 failed [6 != 5]

Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@danpat danpat merged commit b5a4ffe into master Apr 3, 2018
@danpat danpat deleted the datasource_name_annotations branch April 3, 2018 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants