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

Data structure, standard creators, Nicaragua networks #99

Merged
merged 18 commits into from
Jan 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,20 @@ caches on disk for efficient re-use. Then the data is combined with another
source of schedule (time) information in order to create a GTFS file using the
transitfeed library.

**Attention:** The source code is currently very specific to Florianópolis Buses
and Costa Rica Urban Train, but it can be extended to make it work for your use
case. In the config file any transit network can be specified for download from
OpenStreetMap. And by extending the creator classes in code, different
approaches for time information handling can be easily implemented. You can help
with pull requests to improve this script.
For every new city a new [configuration file](https://github.com/grote/osm2gtfs/wiki/Configuration)
needs to be created. Additionally, schedule information should be provided. By-default the schedule information is expected to be provided in a [certain format](https://github.com/grote/osm2gtfs/wiki/Schedule). However other formats are supported through extending the code. For any city and schedule format the script can be easily extended, see the
[developer documentation](https://github.com/grote/osm2gtfs/wiki/Development)
for more information.

Included cities
-----------------

* [Florianópolis, Brazil](./osm2gtfs/creators/fenix/fenix.json)
* [Suburban trains in Costa Rica](./osm2gtfs/creators/incofer/incofer.json)
* [Accra, Ghana](./osm2gtfs/creators/accra/accra.json)
* [Managua, Ciudad Sandino](./osm2gtfs/creators/managua/managua.json) and [Estelí](./osm2gtfs/creators/esteli/esteli.json) in Nicaragua

*Soon, also in your city*

Install
------------
Expand Down
2 changes: 1 addition & 1 deletion osm2gtfs/core/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def write_file(name, content):
if not os.path.isdir('data'):
os.mkdir('data')
with open(os.path.join('data', name), 'wb') as f:
f.write(content.read())
f.write(content)
Copy link
Owner

Choose a reason for hiding this comment

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

@jamescr looks like you are doing the same thing in #120. Once is probably enough.

Copy link
Contributor Author

@pantierra pantierra Jan 23, 2018

Choose a reason for hiding this comment

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

Yes, we just have to coordinate which one to merge in first. I would be happy and propose to give the preference to #120, as this one is smaller and easier to review first and then rebase here.

Copy link
Collaborator

@jamescr jamescr Jan 23, 2018

Choose a reason for hiding this comment

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

I agree doing this once is enough but in configuration.py there is a difference between both Pull Requests (in line 86 of this PR the read() method should be deleted).

The reason why I created the "PR" on master instead to directly in mapanica-master is because this issue is a bug in currently master branch code.

Copy link
Owner

Choose a reason for hiding this comment

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

in configuration.py there is a difference between both Pull Requests (in line 86 of this PR the read() method should be deleted).

@jamescr but doesn't your PR add the read() method there as well? I don't see the difference!?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My PR added the read() in line 84 and do nothing in line 86 (unifying the schedule_source type to str), This PR do nothing on line 84 and adds the read() in line 86.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied the minor change of the read call in the same line as the PR in #120. This way I tested the code and approved the PR there. Either one, #99 or #120 which gets in first will fix the issue in the same way.


@staticmethod
def read_file(name):
Expand Down
9 changes: 5 additions & 4 deletions osm2gtfs/core/configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def get_schedule_source(self, refresh=False):

else:
source_file = self.data['schedule_source']
cached_file = 'schedule-source-' + self.data['selector']
cached_file = self.data['selector'] + '-schedule'

# Preferably return cached data about schedule
if refresh is False:
Expand Down Expand Up @@ -81,11 +81,12 @@ def get_schedule_source(self, refresh=False):
sys.stderr.write(
"Error: Couldn't find schedule_source file.\n")
sys.exit(0)
schedule_source = schedule_source_file
schedule_source = schedule_source_file.read()

# Cache data
Cache.write_file(cached_file, schedule_source)
self._schedule_source = schedule_source

# Cache data
Cache.write_file(cached_file, self._schedule_source)
return self._schedule_source

def _load_config(self, args):
Expand Down
186 changes: 186 additions & 0 deletions osm2gtfs/core/elements.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
# coding=utf-8

import sys
import attr


@attr.s
class Element(object):
"""The basic data element.
Contains the common attributes all other data classes share.

"""
osm_id = attr.ib()
osm_type = attr.ib()
osm_url = attr.ib()

tags = attr.ib()
name = attr.ib()


@attr.s
class Line(Element):
"""A general public transport service Line.

It's a container of meta information and different Itinerary objects for
variants of the same service line.

In OpenStreetMap this is usually represented as "route_master" relation.
In GTFS this is usually represented as "route".

"""
route_id = attr.ib()

route_type = attr.ib(default=None)
route_desc = attr.ib(default=None)
route_color = attr.ib(default="#FFFFFF")
route_text_color = attr.ib(default="#000000")

# Related route variants
_itineraries = attr.ib(default=attr.Factory(list))

def __attrs_post_init__(self):
'''
Populates the object with information obtained from the tags
'''
# pylint: disable=unsupported-membership-test,unsubscriptable-object
if 'colour' in self.tags:
self.route_color = self.tags['colour']

# pylint: disable=unsupported-membership-test,unsubscriptable-object
if 'route_master' in self.tags:
self.route_type = self.tags['route_master'].capitalize()
else:
sys.stderr.write(
"Warning: Route master relation without a route_master tag:\n")
sys.stderr.write(" " + self.osm_url + "\n")

# Try to guess the type differently
if 'route' in self.tags:
self.route_type = self.tags['route'].capitalize()
else:
self.route_type = "Bus"

def add_itinerary(self, itinerary):

if self.route_id != itinerary.route_id:
raise ValueError('Itinerary route ID (' +
itinerary.route_id +
') does not match Line route ID (' +
self.route_id + ')')
# pylint: disable=no-member
self._itineraries.append(itinerary)

def get_itineraries(self):
return self._itineraries


@attr.s
class Itinerary(Element):
"""A public transport service itinerary.

It's a representation of a possible variant of a line, grouped together by
a Line object.

In OpenStreetMap this is usually represented as "route" relation.
In GTFS this is not exlicitly presented but used as base to create "trips"

"""
route_id = attr.ib()
shape = attr.ib()

line = attr.ib(default=None)
fr = attr.ib(default=None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you using fr instead of from ? Don't you think it would be more explicit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also liked from more. Unfortunately it is reserved keyword and can not be used in python. I just didn't touch it in this PR and left it as it was before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

arg, sorry that I've missed that ! let's keep it that way for now ;)

to = attr.ib(default=None)
duration = attr.ib(default=None)

# All stop objects of itinerary
stops = attr.ib(default=attr.Factory(list))

def __attrs_post_init__(self):
'''
Populates the object with information obtained from the tags
'''
# pylint: disable=unsupported-membership-test,unsubscriptable-object
if 'from' in self.tags:
self.fr = self.tags['from']

# pylint: disable=unsupported-membership-test,unsubscriptable-object
if 'to' in self.tags:
self.to = self.tags['to']

def get_stops(self):
return self.stops


@attr.s
class Station(Element):
"""A public transport stop of the type station.

It's a representation of a possible group of stops.

In OpenStreetMap this is usually represented as "stop_area" relation.
In GTFS it is handled as a stop with a location_type=1. Regular Stops with
location_type=0 might specify a station as parent_station.

"""
lat = attr.ib()
lon = attr.ib()

stop_id = attr.ib(default="")
location_type = attr.ib(default=1)

# Stops forming part of this Station
_members = attr.ib(default=attr.Factory(list))

def set_members(self, members):
self._members = members

def get_members(self):
return self._members

def get_stop_id(self):
return self.stop_id

def set_stop_id(self, stop_id):
self.stop_id = stop_id


@attr.s
class Stop(Element):
"""A public transport stop.

In OpenStreetMap this is usually represented as an object of the role
"plattform" in the route.

"""
lat = attr.ib()
lon = attr.ib()

stop_id = attr.ib("")
location_type = attr.ib(default=0)

# The id of the Station this Stop might be part of.
_parent_station = attr.ib(default=None)

def set_parent_station(self, identifier, override=False):
"""
Set the parent_station_id on the first time;
Second attempts throw a warning
"""
if self._parent_station is None or override is True:
self._parent_station = identifier
else:
sys.stderr.write("Warning: Stop is part of two stop areas:\n")
sys.stderr.write(
"https://osm.org/" + self.osm_type + "/" + str(
self.osm_id) + "\n")

def get_parent_station(self):
return self._parent_station

def get_stop_id(self):
return self.stop_id

def set_stop_id(self, stop_id):
self.stop_id = stop_id
Copy link
Owner

Choose a reason for hiding this comment

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

I personally liked how before, the classes were logically grouped and split up in several files instead of one huge file holding all classes like it is here.

Copy link
Contributor Author

@pantierra pantierra Jan 23, 2018

Choose a reason for hiding this comment

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

Oh, yeah, big question on having elements or routes and stops. I also liked routes and stops in different files, until I got the request to abstract the common attributes and I moved them into Element. Then I thought, oh three files: hmmm - no, two files with one class to inherit from - hmmmm, inconsistent, and then chose to put them all, not too different object into one single file. I'm not strong with this one. @grote, what would you suggest?

Copy link
Owner

@grote grote Jan 24, 2018

Choose a reason for hiding this comment

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

Couldn't you just have three files: elements (or osm_base), osm_routes and osm_stops?

The reason why the modules should not be just names routes and stops, is because then variable names in the code shadow module names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this is because of individual taste... I preferred to put them all in one file. But I'm not that strong about this. So, renamed them to elements.py, routes.py, stops.py.

By the way, it doesn't make sense to use the prefix osm_ because the data structure on purpose was made with the motivation to abstract it from OSM particular things instead of having the long spaghetti. All this is living now nicely separated into osm_connector.py.

Copy link
Owner

Choose a reason for hiding this comment

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

Did you see this:

The reason why the modules should not be just names routes and stops, is because then variable names in the code shadow module names.

So in Python it is then ambiguous if routes means the variable or the routes module. Therefore, it is better to give a different name to the module (or rename all stops and routes variables used throughout the code).

Copy link
Contributor Author

@pantierra pantierra Jan 24, 2018

Choose a reason for hiding this comment

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

So, why not staying with one elements.py? To me it seems to be the perfect solution and groups all nicely and understandably.

68 changes: 68 additions & 0 deletions osm2gtfs/core/helper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# coding=utf-8

import sys
from math import cos, sin, atan2, sqrt, radians, degrees


class Helper(object):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great idea with the Helper class!

"""The Helper class contains useful static functions

"""

@staticmethod
def print_shape_for_leaflet(shape):
print "var shape = [",
i = 0
for node in shape:
print "new L.LatLng(" + str(node["lat"]) + ", " + str(node["lon"]) + ")",
if i != len(shape) - 1:
print ",",
i += 1
print "];"
i = 0
for node in shape:
print "L.marker([" + str(node["lat"]) + ", " + str(node["lon"]) + "]).addTo(map)"
print " .bindPopup(\"" + str(i) + "\").openPopup();"
i += 1

@staticmethod
def get_center_of_nodes(nodes):
"""Helper function to get center coordinates of a group of nodes

"""
x = 0
y = 0
z = 0

if len(nodes) < 1:
sys.stderr.write("Cannot find the center of zero nodes\n")
for node in nodes:
lat = radians(float(node.lat))
lon = radians(float(node.lon))

x += cos(lat) * cos(lon)
y += cos(lat) * sin(lon)
z += sin(lat)

x = float(x / len(nodes))
y = float(y / len(nodes))
z = float(z / len(nodes))

center_lat = degrees(atan2(z, sqrt(x * x + y * y)))
center_lon = degrees(atan2(y, x))

return center_lat, center_lon

@staticmethod
def interpolate_stop_times(trip):
"""
Interpolate stop_times, because Navitia does not handle this itself
"""
try:
for secs, stop_time, is_timepoint in trip.GetTimeInterpolatedStops():
if not is_timepoint:
stop_time.arrival_secs = secs
stop_time.departure_secs = secs
trip.ReplaceStopTimeObject(stop_time)
except ValueError as e:
print(e)
Loading