search

usdot-jpo-ode / wzdx

The Work Zone Data Exchange (WZDx) Specification aims to make harmonized work zone data provided by infrastructure owners and operators (IOOs) available for third party use, making travel on public roads safer and more efficient through ubiquitous access to data on work zone activity.
Creative Commons Zero v1.0 Universal
92 stars 62 forks source link

Add CDS reference #326

Closed jlarsonOmahaNE closed 1 year ago

jlarsonOmahaNE commented 2 years ago

This Pull request address #241 and will add a Curb Data Spec curb_zone reference for work zones.

schnuerle commented 2 years ago

We've added a similar reference to WZDx from CDS here: https://github.com/openmobilityfoundation/curb-data-specification/pull/96

j-d-b commented 2 years ago

@jlarsonOmahaNE the time frame for changes for v4.1 has closed, so this cannot be implemented for v4.1.

jlarsonOmahaNE commented 2 years ago

@j-d-b Just following Nate's recommendation to open a PR

natedeshmukhtowery commented 2 years ago

I knew we were approaching the end of the v4.1 cycle but assumed that since there are several significant PRs outstanding, this minor addition that had seen a fair amount of discussion as an issue would be fine to include.

j-d-b commented 2 years ago

@jlarsonOmahaNE you can always open a PR at any time! Thank you for doing it. The target branch just shouldn't be release/v4.1 in this case.

@natedeshmukhtowery given that there are no more subgroup meetings, we are past the 4.1 cutoff, and we never discussed the issue (let alone the PR) with the members or subgroup co-chairs, the process for the changes to be implemented was not followed, so I don't think it can be included in this release.

jlarsonOmahaNE commented 2 years ago

The target branch just shouldn't be release/v4.1 in this case.

@j-d-b Should I open a new branch for "future integration" and keep this PR? Or remove this PR but keep the feature/cds branch?

j-d-b commented 2 years ago

Should I open a new branch for "future integration" and keep this PR? Or remove this PR but keep the feature/cds branch?

@jlarsonOmahaNE we don't have a defined process in place for that, so I think it's simplest to just target main as the base. You may want to convert this PR to a draft until release/v4.1 is merged to main, as it can't be merged before then.

Also, based on discussion with @natedeshmukhtowery, though this can't go in for v4.1, it may be able to go in by the end of 2022 in a short-cycle 4.2 release.

jlarsonOmahaNE commented 2 years ago

we don't have a defined process in place for that, so I think it's simplest to just target main as the base. You may want to convert this PR to a draft until release/v4.1 is merged to main, as it can't be merged before then.

Sounds good, I will make it a draft. Thank you!

schnuerle commented 2 years ago

See PR #342 pushing to feature/cds branch for resolution of most of the suggestions made by @j-d-b

j-d-b commented 2 years ago

The release/v4.2 branch needs to be merged into feature/cds and the resulting merge conflicts resolved.

mark-mockett commented 2 years ago

I'll add an official review once the v4.2 release branch is updated and merged back in, but this looks good overall to me. My two high-level questions are:

I don't mean to suggest there are changes that need to be made, just trying to understand the logic behind the proposed implementation.

schnuerle commented 2 years ago
  • Should be a way to define what the impact is to each curb zone on the CdsCurbZoneReference object? Right now it just identifies which zone is impacted

I think this is a good question. If there is a mechanism or field in WZDx that can describe an impact in some way, maybe that field could be added here. But I do think it's good just to know there even is an impact!

  • How scalable is this implementation to a large number of curb zones? It seems like it might be redundant to list the feed url multiple times for a lot of curb zones in the same feed (which could be addressed by allowing an array of CZ id's), while still allowing multiple CdsCurbZoneReference objects in case multiple CDS APIs are being referenced

I've thought about that too if there are many impacts, and there was a comment about that early on. I think it's ok as is for now because my guess is the initial use cases in cities will have only a handful of CDS curb zones defined. Maybe if it becomes redundant in some real-world uses we can look at making it more array based in a future release.

jlarsonOmahaNE commented 2 years ago

Per discussion in the spec-update co-chairs meeting 2022-10-21, changes will be made by:

  1. The curb_zone_id from string to array to allow for multiple ids to be referenced. Would become curb_zone_ids.
  2. The cds_curbs_api_url will only need to be provided once and not for each curb_zone in the array.