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

Add MDS Stops RT Specification (Agency & Provider) #427

Merged
merged 27 commits into from
Jun 29, 2020

Conversation

avatarneil
Copy link
Contributor

@avatarneil avatarneil commented Jan 16, 2020

Explain pull request

This PR is a work in progress to resolve #374 .

Is this a breaking change

A breaking change would require consumers or implementors of the API to modify their code for it to continue to function (ex: renaming of a required field or the change in data type of an existing field). A non-breaking change would allow existing code to continue to function (ex: addition of an optional field or the creation of a new optional endpoint).

  • No, not breaking

Impacted Spec

Which spec(s) will this pull request impact?

  • Provider
  • Agency

Additional context

Stops are intended to be a strict superset of GTFS/GBFS -- keep that in mind when seeing some fields that are unnecessary for the base scope of MDS currently.

@avatarneil avatarneil requested a review from a team January 16, 2020 17:41
@avatarneil avatarneil requested a review from a team as a code owner January 16, 2020 17:41
@avatarneil avatarneil requested a review from a team January 16, 2020 17:41
@dirkdk
Copy link
Contributor

dirkdk commented Jan 16, 2020

quick question: how would stops with charging capabilities be reflected?

Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

More of a nitpick, but I would rather see this PR focused on the specific changes for docks/stops/etc. and save all the formatting fixes elsewhere in the doc for another cleanup PR. This makes review a lot easier, and tracing the history becomes less cumbersome when we can see exactly why a particular line was changed.

@avatarneil
Copy link
Contributor Author

@thekaveman Resolved your previous nit. Also cleaned up the commit history a bit in the process.

@avatarneil avatarneil changed the title MDS-Agency and MDS-Provider Stops Specification MDS-Provider Stops Specification Jan 23, 2020
@avatarneil
Copy link
Contributor Author

@thekaveman I've also removed any agency changes that were included in this, and have cut a separate PR.

@dirkdk
Copy link
Contributor

dirkdk commented Feb 6, 2020

I guess this PR does not define vehicle_type yet. In order to support charging, we could define them as following with the _charging postfix:

[
num_bike_human_available, 
num_bike_electric_assist_available, 
num_bike_electric_assist_available_charging, 
num_scooter_electric_available, 
num_scooter_electric_available_charging
]

e.g.:

{
  num_bike_human_available: 10,
  num_bike_electric_assist_available: 5,
  num_bike_electric_assist_available_charging: 3,
  num_scooter_electric_available: 3,
  num_scooter_electric_available_charging: 2
}

@avatarneil
Copy link
Contributor Author

avatarneil commented Feb 13, 2020

@dirkdk Vehicle Types are defined here https://github.com/openmobilityfoundation/mobility-data-specification/tree/dev/provider#vehicle-types already, I can add a hyperlink to the line that references that enum.

@dirkdk
Copy link
Contributor

dirkdk commented Feb 13, 2020

@dirkdk Vehicle Types are defined here https://github.com/openmobilityfoundation/mobility-data-specification/tree/dev/provider#vehicle-types already, I can add a hyperlink to the line that references that enum.

yes thinking if we need to include propulsion_type. Definitely we need to distinct non-charging racks from charging racks

@jiffyclub
Copy link
Contributor

I'm not as familiar with GBFS so this might not be compatible with that, but instead of listing vehicle counts and having to invent increasingly complex keys for that I'd rather see a list of vehicles with associated attributes so end users can group and count those as needed.

@thekaveman thekaveman modified the milestones: 0.4.1, Future Feb 13, 2020
@jfh01
Copy link
Contributor

jfh01 commented Mar 20, 2020

There are some relevant changes likely coming to GBFS here: MobilityData/gbfs#219

Worth making sure we have alignment with those (especially around valet and virtual stations) in whatever we implement.

@jfh01 jfh01 modified the milestones: Future, 1.0.0 Apr 9, 2020
@vperron
Copy link

vperron commented May 12, 2020

Hi, I must say that I like this approach very much since:

  • it's a pragmatic one that aims to support a reasonable level of information from the GBFS standard
  • it's generic enough to be able to evolve and support dockless mobility in future versions (for instance it already supports various vehicle types and expected geography IDs for virtual stations)
  • has a good chance to be integrated into the 1.0.0 milestone without too much hassle, especially if the changes to /trips and /status_changes are out of scope for 1.0 so far.

I'm not that sure about the wheelchair_boarding boolean though, I think it's be more generic to have a list of "available services" that we can extend, much like the propulsion_types for instance.

However, I think that #441, while maybe going maybe too far for a first step in my opinion, also makes some very interesting points, most notably:

  • it provides an optional gbfs_station_id: being able to reconcile MDS info and GBFS can be a very big plus for some peeps who would want to use both approaches.
  • a status field that reports the whole station status: it happens a lot that some stations are disabled, decommissioned, temporarily closed, ...
  • a reported_at field: we need to be able to know, in a real-time endpoint, how stale this information can be. I think this is really important.

I also think that their approach with arrays of MDS-compatible vehicle types, propulsion types, vehicles and docks is cleaner and more informative than multiple dictionaries of vehicles and spaces counts, which sounds very GBFS-y :)

So to sum it up I'd say that in my opinion, we can find a best of both worlds by merging some of the fields in that other PR with this one and still be generic and simple enough for a first approach.

@jfh01
Copy link
Contributor

jfh01 commented Jun 4, 2020

Here is a doc we put together with MobilityData and NABSA to provide additional guidance on how GBFS and MDS should align with each other:

https://github.com/openmobilityfoundation/governance/blob/master/technical/GBFS_and_MDS.md

@avatarneil
Copy link
Contributor Author

@vperron I agree, having just a boolean for wheelchair_boarding feels wrong, and also not super applicable for micro-mobility (unless there are handicap accessible scooters). How about an accessibility_options array that (for now) would just be empty? I can certainly imagine that this would be applicable for vehicles as well (especially for non-micro-mobility modes).

Agreed having a pointer to gbfs_station_id could be beneficial, though I think that while it works well for the Provider API, it would be tricky for the Agency API, as for a given Stop I believe there could be multiple gbfs_station_ids for the same logical Stop (because Providers have their own GBFS feeds). This may be a non-issue for GBFS, however I know for GTFS there is certainly fragmentation. Even though this specific PR is for Provider, I'd really like to keep the Provider and Agency definitions of Stops aligned as much as possible.

The status field I think could be beneficial, totally onboard with that! I'm thinking we could just lift the enum definition from #441.

The reported_at field definitely makes sense, however I'd prefer to name it last_updated -- this aligns with the RT /vehicles endpoint in Provider.

I'd be okay with switching from using the mapped values to a more explicit array of docks/vehicles, however I'm not sure that's entirely necessary (and may have some downsides). Having the maps certainly makes deriving counts of certain vehicle types at the Stops faster (constant time lookup as opposed to having to iterate through the entire array), and explicitly encoding an array of vehicles (with essentially all of the information for the vehicle) within the stop seems like duplicating data that is already available via the /vehicles endpoint to me. My counter proposal is, instead of dumping the maps, what if we kept them and additionally included a vehicle_ids array which would contain pointers to the vehicles which are currently at the stop? This would then give all of the information needed to be able to derive specific details about the vehicles which are at the stop from the /vehicles endpoint, without duplicating in the vehicle data.

also use link references
elevate 5-minute requirement from /vehicles to this section

use reference links
@schnuerle
Copy link
Member

schnuerle commented Jun 27, 2020

@thekaveman thank you for the rebase!

It looks like 'spaces' is the consensus term to replace 'spots' with 4 votes.

@avatarneil what happened to the 'Place' option, or did I just make that up?

Let's also talk about the differences between MDS and GBFS here.

  1. station, docks, bikes vs stop, spaces, vehicles - I think these differences are ok, since MDS has a broader scope that GBFS and some of the GBFS naming may be a bit of a relic despite GBFS supporting more vehicle types. Will GBFS change this wording eventually, or keep as is? I'm not sure.
  2. How does 'stop status' work in MDS? Is it an array of the 3 options with a boolean for each? It's not clear to me looking at the spec.
  3. Is there a reason to keep MDS stops as one endpoint, or should it be broken out into stop_status and stop_information, like in GBFS? I assume this is done to reduce the payload size and split up items that change less frequently. With MDS it may make sense to keep as is since this info may be expected to change more frequently.
  4. MDS is using a GeoJSON Point Feature for location, vs a lat/lon point, which makes sense to me for consistency within MDS. However, should this be a geography instead of a point? I think we discussed this on calls.
  5. Should stop_name be changed to name to match GBFS and be more generic?
  6. Note MDS is introducing the idea of parent stops (a hierarchy) and adding vehicle_ids (which makes sense for authenticated MDS).
GBFS station_status MDS stops
station_id stop_id
num_docks_available num_spaces_available
num_docks_disabled num_spaces_disabled
num_bikes_available num_vehicles_available
num_bikes_disabled num_vehicles_disabled
is_installed stop status is_installed
is_renting stop status is_renting
is_returning stop status is_returning
last_reported last_reported
GBFS station_information MDS stops
station_id stop_id
lat location
lon location
name stop_name
short_name short_name
address address
cross_street cross_street
region_id region_id
post_code post_code
rental_methods rental_methods
capacity capacity
X parent_stop
X vehicle_ids

Thoughts on changes here are welcome, even though the release candidate will be made this Monday. During the release approval process the OMF Tech Council and Board can discuss tweaks to this proposal, and comments made here will help influence those decisions.

@avatarneil
Copy link
Contributor Author

It looks like 'spaces' is the consensus term to replace 'spots' with 4 votes.

I'll replace 'spots' with 'spaces'.

@avatarneil what happened to the 'Place' option, or did I just make that up?

'Place' totally slipped my mind, but I do remember that from the call -- don't think you made it up :)

  1. How does 'stop status' work in MDS? Is it an array of the 3 options with a boolean for each? It's not clear to me looking at the spec.

It's an object with the properties, e.g.

{
  "is_installed": true,
  "is_renting": false,
  "is_returning": true,
}
  1. Is there a reason to keep MDS stops as one endpoint, or should it be broken out into stop_status and stop_information, like in GBFS? I assume this is done to reduce the payload size and split up items that change less frequently. With MDS it may make sense to keep as is since this info may be expected to change more frequently.

I think we ought to keep it as one endpoint (at least for now) -- this aligns well with how we report on vehicles in MDS.

  1. MDS is using a GeoJSON Point Feature for location, vs a lat/lon point, which makes sense to me for consistency within MDS. However, should this be a geography instead of a point? I think we discussed this on calls.

This confusion is one of the reasons why I was pushing for having simple lat/lng properties in the object, and using Geography (via a geography_id) for any GeoJSON. What we have now, is both location represented as a GeoJSON Point Feature, as well as supporting a linkage to a Geography via a geography_id (allowing a full GeoJSON Feature Collection), which is a little odd.

  1. Should stop_name be changed to name to match GBFS and be more generic?

That sounds fine by me.

Cc. @schnuerle

@schnuerle
Copy link
Member

Thank you for the updates and clarifications @avatarneil!

I think this PR is ready for the 1.0.0 Release Candidate today.

For discussion during the review process:

  1. Should we use 'place' instead of 'space'? Mostly because place starts with 'P' so would be easier to say along with with word 'Stops', and 'space' subtly references cars with 'parking space'.
  2. We have not had a case of multiple IDs in an array like we do with the 'devices' field here. Should these fields instead have '_ids' at the end, like GBFS does with 'station_ids' and 'region_ids'? Either way this is starting a naming standard for MDS.
  3. Should we intentionally explain our naming of stop/spaces/vehicles, or should we instead align with GBFS station/docks/bikes even though the terminology is narrower? We can ask MobilityData and GBFS community if there is any intention to broaden in the future.

@schnuerle schnuerle self-requested a review June 29, 2020 17:27
Copy link
Member

@schnuerle schnuerle left a comment

Choose a reason for hiding this comment

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

Approved for the 1.0.0 release candidate.

@avatarneil avatarneil requested a review from Retzoh June 29, 2020 17:30
@schnuerle schnuerle merged commit 8912442 into openmobilityfoundation:dev Jun 29, 2020
@schnuerle
Copy link
Member

Some updates from the Provider Services call today:

  1. Should we use 'place' instead of 'space'? Mostly because place starts with 'P' so would be easier to say along with with word 'Stops', and 'space' subtly references cars with 'parking space'.

No opinion one way or the other. I like "Place."

  1. Should we intentionally explain our naming of stop/spaces/vehicles, or should we instead align with GBFS station/docks/bikes even though the terminology is narrower? We can ask MobilityData and GBFS community if there is any intention to broaden in the future.

Heidi Guenin with MobilityData who runs GBFS said that there is a PR to change bikes to vehicles. Car sharing is a new use case. Bottom line is don't align to GBFS terminology right now.

@mdurling
Copy link

mdurling commented Jul 9, 2020

@schnuerle Have we considered position, that is, stops and stop positions?

@schnuerle
Copy link
Member

@schnuerle Have we considered position, that is, stops and stop positions?

Yes we did, it was in the poll we took (though Place was missing) and didn't get any votes.

Based on the feedback/votes on the City Services call today and the Provider Services call last week, I'm going to rename Spaces to Places in the release candidate PR #544.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agency Specific to the Agency API enhancement New feature or request Provider Specific to the Provider API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify /status_changes to represent docked event locations Docked Bike Share Systems