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.

Sign up for GitHub

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

Jump to bottom

WIP: Data structure 2.0 #96

Closed
wants to merge 14 commits into from
Closed

WIP: Data structure 2.0 #96

wants to merge 14 commits into from

Conversation

xamanu
Copy link
Contributor

@xamanu xamanu commented Dec 7, 2017 •
edited
Loading

This is a new pull request for the overhauled data structure #30 with a lot of wonderful work by @AltNico
It is similar to #89 (closing now), but rebased on #91, which seems to be the next to get in.

@xamanu
Copy link
Contributor Author

xamanu commented Dec 7, 2017

The CI pipeline will fail, because of an error risen by pylint is related to attr.s and can not be fixed: python-attrs/attrs#215

@xamanu
Copy link
Contributor Author

xamanu commented Dec 7, 2017 •
edited
Loading

@nlehuby and @prhod, may I request your help on this one? This is a wonderful but deep change and I think it will be difficult to finalize it without your support.

Status on Accra:

  • It builds (yeah!)
  • Accra's unit tests fail because of File "osm2gtfs/tests/accra_unittests.py", line 151, in test_gtfs_from_cache 'The generated GTFS is different from the expected one')
  • Indeed the GTFS provided for the tests (accra_tests.zip.ref) is 1.1M, while the generated one (accra_tests.zip) is almost half: 602K

Status on Fenix:

  • Throws error: AttributeError: 'Line' object has no attribute 'set_duration'. Need to look into that.

Status on Nicaragua:

  • We use this structure already to generate our GTFSes, but in a different branch without any rebasing. Needs to be further tested.

@xamanu xamanu changed the title Data structure 2.0 WIP: Data structure 2.0 Dec 7, 2017
This was referenced Dec 7, 2017
Closed
Merged
@xamanu
Copy link
Contributor Author

xamanu commented Dec 7, 2017

I changed the cache file names a bit, so this PR can coexists with caching with the old version (before this PR). No need to delete cached files.

This was referenced Dec 8, 2017
Merged
Closed
@xamanu
Copy link
Contributor Author

xamanu commented Dec 8, 2017 •
edited
Loading

Working on Fenix with the new data structure and to improve the Itinerary class, the use of travel_time and duration was unified and generally improved. Now all kind of tags of the routes/itineraries are available in the creators (#76).

@xamanu xamanu mentioned this pull request Dec 9, 2017
Closed
nlehuby
nlehuby requested changes Dec 9, 2017
View reviewed changes
Copy link
Collaborator

@nlehuby nlehuby left a comment

Choose a reason for hiding this comment

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

The shape_id is different with this new version, this may explain partly the size difference.

osm2gtfs/core/routes.py Outdated
self.name = self.tags['name']

if "colour" in self.tags:
self.route_color = OsmConnector.get_hex_code_for_color(
Copy link
Collaborator

Choose a reason for hiding this comment

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

you need to import OsmConnector from osm2gtfs.core.osm_connector

osm2gtfs/creators/accra/trips_creator_accra.py
trip_gtfs.shape_id = TripsCreator.add_shape(
schedule, a_route_ref, a_route)
feed, a_route.route_id, a_route)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you need to replace a_route.route.id by a_route.osm_id to keep the shape_id identical.

osm2gtfs/tests/accra_unittests.py
@@ -100,8 +100,8 @@ def test_refresh_routes_cache(self):
self.assertEqual(len(routes), 277, 'Wrong count of routes in the cache file')
Copy link
Collaborator

Choose a reason for hiding this comment

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

you need to change the name of the cache file here too (routes-accra to accra-routes)

osm2gtfs/tests/accra_unittests.py
@@ -117,32 +117,32 @@ def test_refresh_stops_cache(self):

Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

@xamanu
Copy link
Contributor Author

xamanu commented Dec 9, 2017

Thanks for the review, @nlehuby. I adjusted the things you mentioned. The whole state is currently still moveing in things from the other branch and make it fit together. Probably you want to wait, until we got it to a stage where we are more confident asking you for help again.

@xamanu xamanu mentioned this pull request Dec 10, 2017
Closed
@xamanu
Copy link
Contributor Author

xamanu commented Dec 11, 2017

I will close this PR, as it is still not ready, yet, and I don't want to spam you (more) with push notifications. We will re-open, once we are in a shape that we would kindly ask for a review. Thanks.

In case you want to support the ongoing development please have a look on this branch: https://github.com/mapanica/osm2gtfs/tree/data-structure-rebased

@xamanu xamanu closed this Dec 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlehuby nlehuby nlehuby requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@xamanu @nlehuby @AltNico @ialokim