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

Aggregate available vehicle_types at a station #261

Merged
merged 8 commits into from
Oct 14, 2020

Conversation

sfaubert1
Copy link
Contributor

Based on conversations in issue #250

Modified station_status.json to aggregate the number of available vehicles per vehicle_type and removed the bike_id, is_reserved, is_disabled, vehicle_type_id and current_range_meters from the feed since they're available in free_bike_status.json.

Also modified free_bike_status.json to specify the station_id where the vehicle is, in order to provide more details on the vehicle (such as 'current_range_meters'). So the free_bike_status.json could be used for free floating vehicles and docked vehicles as well to provide more information on each of the vehicles.

Based on conversations in issue MobilityData#250 

Modified station_status.json to aggregate the number of available vehicles per vehicle_type and removed the `bike_id`, `is_reserved`, `is_disabled`, `vehicle_type_id` and `current_range_meters` from the feed since they're available in free_bike_status.json. 

Also modified free_bike_status.json to specify the station_id where the vehicle is, in order to provide more details on the vehicle (such as 'current_range_meters'). So the free_bike_status.json could be used for free floating vehicles and docked vehicles as well to provide more information on each of the vehicles.
@matt-wirtz
Copy link

The proposal sounds logical but includes some fundamental changes to the semantic.

So far the endpoint free_bike_status.json referred to free floating bikes. Bikes that are not docked or locked at a station. So the free in free_bike_status had the meaning of free floating rather then available to rent. But with this proposal the free now means available to rent. So I think this is a breaking change and should be labeled as this. Primarily because available docked bikes will no longer show up at the endpoint station_status.json but rather at free_bile_status.json.

And following this argumentation it would make sense to rename free_bike_status.json into bike_status.json. Bike_status.json would thus become a counterpart to station_status.json.

Although I can follow the basic idea of this PR I don't see a significant advantage for providing or consuming apps of this PR right away? What would be the most important benefits of this breaking PR?

@sfaubert1
Copy link
Contributor Author

@matt-wirtz thanks for you comment. The goal of this PR is to reduce the size of the station_status.json feed for large systems, by excluding the bike details and go back to an aggregated number of available vehicles in this feed. See issue #250 for more information.

The number of available docked bikes would still be in station_status.json in vehicle_types_available. Only the details of each bike that would be removed from station_status.json and added in free_bike_status.json. It was proposed in issue #250 to refactor the free_bike_status.json to include all bikes and their details, but I didn't change the name in this PR thinking it would be a breaking change. I agree that for better comprehension, we could change the name of free_bike_status.json to bike_status.json.

@davidlewis-ito, @kanagy, @mplsmitch, since involved in the original issue leading to this PR, just thought you would be interested to see the proposed changes.

Fixed a typo (missing quote)
@mplsmitch
Copy link
Collaborator

@matt-wirtz The intent of free_bike_status.json endpoint has been to describe vehicles that are available for rent, but not at a station. In other works these are free floating or dockless vehicles that are currently available for rent. This PR maintains the distinction between docked and dockless but consolidates all information relating to individual vehicles in a single file. We're currently working and a plan to incorporate non-breaking changes from this PR into a minor release with any breaking and additional changes like renaming of free_bike_status.json happening in the next major release.

@matt-wirtz
Copy link

@sfaubert1 Thanks for pointing again to issue #250. I kind of overlooked the reference and directly read through the proposed changes. So by removing the vehicles array in the station_status endpoint roughly 100 bytes (uncompressed) are saved per bike. With e.g. 5.000 bikes this would roughly be 500kb.

@mplsmitch Maybe I'm overlooking something but I still don't see how this PR should be non-breaking. Let's say there is a consumer app who uses the information of the station_information and station_status endpoints to display to the user all bikes available at stations including their e.g. current_range_meters. Since this PR removes the vehicles array from the endpoint station_status the app would not be able the list the individual bikes available without changes to their code. So from my understanding it's a breaking change.

gbfs.md Outdated
&emsp;\- `is_disabled` <br/>*(added in v2.1-RC)* | Yes | Boolean | Is the vehicle currently disabled (broken)
&emsp;\- `vehicle_type_id` <br/>*(added in v2.1-RC)* | Yes | ID | The vehicle_type_id of this vehicle as described in [vehicle_types.json](#vehicle_typesjson).
&emsp;\- `current_range_meters` <br/>*(added in v2.1-RC)* | Conditionally Required | Non-negative float | If the corresponding vehicle_type definition for this vehicle has a motor, then this field is required. This value represents the furthest distance in meters that the vehicle can travel without recharging or refueling with the vehicle's current charge or fuel.
\- `vehicle_types_available` <br/>*(added in v2.1-RC)* | Conditionally Required | Object | This field is required if the [vehicle_types.json](#vehicle_typesjson) file has been defined. This field is an object where each key is a `vehicle_type_id` as described in [vehicle_types.json](#vehicle_typesjson) and the value is a number representing the total number of available vehicle at this station for the specified vehicle type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

vehicles should be plural: available vehicles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now changed, thanks for letting me know

gbfs.md Outdated
@@ -450,15 +431,15 @@ Example:

### free_bike_status.json

Describes vehicles that are not at a station and are not currently in the middle of an active ride.
Describes all vehicles that are not at a station and are not currently in the middle of an active ride. From v2.1-RC, also provides more information about vehicles that are at a station if the [vehicle_types.json](#vehicle_typesjson) file has been defined.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would like to clarify this definition and add line about vehicles in active rental from Best Practices. Suggesting this:

"Describes all vehicles that are not currently in active rental. Required of free floating (dockless) vehicles. Conditionally required of station based (docked) vehicles when vehicle_types.json has been defined. Vehicles that are part of an active rental must not appear in this feed."

Copy link
Collaborator

Choose a reason for hiding this comment

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

This definition should also be updated under "Files" above on line 71.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's more clear, thanks! Updated at both places.

@mplsmitch
Copy link
Collaborator

Need to remove the reference to station_status.json in the definition of vehicle_types.json under "Files" on line 68

- updated free_bike_status.json description (2 places)
- corrected a typo ("available vehicles")
@sfaubert1
Copy link
Contributor Author

About previous comment:

Need to remove the reference to station_status.json in the definition of vehicle_types.json under "Files" on line 68

Please correct me if I'm wrong, but I think the reference to station_status.json in the definition of vehicle_types.json should remain since there would still be references to the vehicle types in the vehicle_types_available field of station_status.json (so the vehicle_types.json would be required if used in station_status.json).

@mplsmitch
Copy link
Collaborator

@sfaubert1 You are correct - the reference to station_status.json should remain in the vehicle_types.json definition.

@sfaubert1
Copy link
Contributor Author

I hereby call a vote on this proposal. Voting will be open for 7 full days, until 11:59PM UTC on September 23rd.

Please vote for or against the proposal, and include the organization for which you are voting in your comment.

Please note if you can commit to implementing the proposal.

@sfaubert1
Copy link
Contributor Author

+1 from PBSC :)

and also committing to implement

@kanagy
Copy link

kanagy commented Sep 16, 2020

Nits:

  • there's a typo in free_bike_status.json description: "Conditionally required of station based (docked) vehicles when vehicle_types.json has been defined.". of --> for.
  • the indentation is off in some of the examples for some of the added lines.
  • Should vehicle_types_available description follow the same style as vehicle_docks_available? I.e. vehicle_docks_available describes in the table the object fields vehicle_type_ids and count. vehicle_types_available should describe those too to be consistent.

Question: Why is docked free_bike_status conditioned on vehicle_types.json being defined? I think free_bikes could describe the vehicles at a station even when the system doesn't define different vehicle_types.

Otherwise, in principle this looks good to me (Google Maps), but we don't plan to implement this soon. In particular I think this is a good direction for supporting hybrid systems where bikes can be both dockless and docked.

@davidlewis-ito
Copy link

+1 from itoworld

We will be consuming and producing

Updated based on comments from @kanagy
Fixed typo in free_bike_status description, fixed indentation in examples, matched style of vehicle_docks_available in vehicle_types_available.
@sfaubert1
Copy link
Contributor Author

Thanks @mplsmitch for the updates!

For the description of vehicle_types_available that was changed to match the format of vehicle_docks_available, is there a reason why we should keep an array for vehicle_type_ids?

For us the count of available bike will always be for one type of vehicle only; in the event of a case where multiple vehicle types would have the same count, the meaning could be confusing, for example:

        "vehicle_types_available": [{
          "vehicle_type_ids": ["abc123", "xyz789"],
          "count": 2
        }, {
          "vehicle_type_ids": ["def456"],
          "count": 4
        }]

In the example above, does it mean there's 2 vehicles of type "abc123" and 2 of type "xyz789" available as well or something else?

Since vehicle_type_capacity in station_information.json was also an object mapping the vehicle type to its count, we thought it would be easy to understand for vehicle_types_available. For example:

        "vehicle_types_available": {
          "abc123": 2,
          "xyz789": 2,
          "def456": 4
        }, 

If we have to go with an array of objects, maybe consider a single string instead of an array for vehicle_type_ids (or vehicle_type_id since only one). For example:

        "vehicle_types_available": [{
          "vehicle_type_id": "abc123",
          "count": 2
        }, {
          "vehicle_type_id": "xyz789",
          "count": 2
        }, {
          "vehicle_type_ids": "def456",
          "count": 4
        }]

The advantage of this last example would be if we need to include more properties to the vehicle_types_available field in the future, but it would be more heavy for sure.

@mplsmitch
Copy link
Collaborator

@sfaubert1 Thanks for catching that - the intention was to make it a single string like in your 3rd example as was discussed in #250 . I'll make the change.

vehicle_type_id becomes string
@yocontra
Copy link
Contributor

+1 from Stae

Copy link

@kanagy kanagy left a comment

Choose a reason for hiding this comment

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

Following up on previous comments and also noticed few typos.

gbfs.md Outdated Show resolved Hide resolved
gbfs.md Outdated Show resolved Hide resolved
gbfs.md Show resolved Hide resolved
gbfs.md Outdated Show resolved Hide resolved
gbfs.md Outdated Show resolved Hide resolved
Fix typos
Update definition of free_bike_status.json
@nighthawk
Copy link

+1 from SkedGo

We will be consuming

@cmonagle
Copy link
Contributor

+1 from Transit

@kanagy
Copy link

kanagy commented Sep 24, 2020

+1 from Google Maps

@heidiguenin
Copy link
Contributor

The vote on this is now closed, and it passes!

Votes in favor:
PBSC (producer)
ItoWorld (consumer)
Stae (consumer)
SkedGo (consumer)
Transit (consumer)
Google Maps (consumer)

No votes against.

We'll be merging, creating v2.2RC (likely after the vote on pricing!), and then we'll follow up with those of you who said you'd be implementing. Thanks for all of your help making sure the new dockless support is implementable!

@barbeau barbeau merged commit de67c7a into MobilityData:master Oct 14, 2020
mplsmitch pushed a commit that referenced this pull request Oct 23, 2020
minor edits, updates stations_status to align with changes in #261
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.