Added bike theft data - #47
Conversation
tallcoleman
left a comment
There was a problem hiding this comment.
Thanks for putting this together - I’m very excited to get a working bike theft map up on the site!
I’ve left some review comments for things to look at. My goal is to help get this dataset into the pipeline as soon as possible to enable the frontend to go live as soon as it’s ready. I’ve organized my comments into two sections: “review now” should be resolved in this PR, and “review before frontend is published” can be done in a future PR. Most other comments should be optional.
Review now
- The logic in
bike-thefts-etl.pycan be consolidated intoupdate_bike_thefts.pyso that you just have one script to call from the github action. - The preferred way to work with downloaded data is with pandas/geopandas. There are several utility functions in the shared folders that help with common tasks like saving a geopandas dataframe to a geojson file or downloading a geojson file from the open data portal.
- The ideal output file format is
.geojson; the MapLibre library that we use natively handles it well in an optimized way. - The output files should be saved in the folder
bike_theft/that the github action cherry picks over to thedatabranch on each run. Output files should generally not be saved to themainbranch. - One exception to that: I think it would be helpful to save the documentation PDFs that are currently linked from the open data portal since they serve as a reference for some of the dicts used in the ETL processing.
- A StatusManager instance should be added to track the metadata every time the data is updated. Lines 76-79, 93, and 133-139 in https://github.com/bikespace/parking-map-data/blob/main/src/bikespace_data/bicycle_network/update_cycling_network.py should give you a good example of usage.
- Once you have a StatusManager instance set up, you can use it to check if the source dataset has been updated and skip the rest of the ETL if it has not. This check/skip should be added since the dataset is only updated annually, but the script will run every day.
Review before frontend is published
- To prevent upstream changes to the data format from causing unanticpated script behaviour, I would reccommend adding validation for your source data using pandera (https://pandera.readthedocs.io/en/stable/). You can see a good example of this https://github.com/bikespace/parking-map-data/blob/main/src/bikespace_data/bicycle_network/update_cycling_network.py with how
cycling_network_schemaandcycling_network_schema_optionalare used. This allows you to fail the processing script if e.g. an important column is removed or re-named in the source data and/or to log non-critical variances (e.g. a new location type is added). - I would recommend trying to add at least one or two unit tests. Doing test cases for one of the functions like
normalize_colorwould be a relatively easy way to start. Running unit tests with coverage will help you catch subtle bugs in your logic flow, ensure that edge cases are handled properly, and gives a lot more confidence when you’re updating or building on the codebase later.
One other thing to think about for the future, is that each point represents the closest intersection to the location of the bike theft. It would be interesting to line this dataset up against the City’s intersection dataset (https://open.toronto.ca/dataset/intersection-file-city-of-toronto/) to see if there are any intersections where there are no bike thefts reported nearby.
|
Also FYI I ran our manual action to generate a preview: https://659fa6e7.bikespace-v2.pages.dev/stolen-history-map |
thesun7385
left a comment
There was a problem hiding this comment.
@tallcoleman I just updated the function to save the file in geojson, and I also revised the job update bike stolen history data as well. Feel free to have a look!
tallcoleman
left a comment
There was a problem hiding this comment.
Thanks for making these updates! I've left some additional comments and once you've gone through those, we should be ready to merge.
I realized I should add better contributor documentation; I've opened pr 49 to do that and you can see the draft documentation here. Take a look through and let me know if there's anything there that could be improved?
For the final PR, please make sure that it only includes the source files and does not include the .geojson or status manager file outputs.
It's a bit tricky during development because you can't .gitignore the outputs (otherwise the data update action wouldn't work). If you want to be able to run the script and see the output files during development without having git track them and without breaking the update action, you can add the output folder to your local .git/info/exclude.
thesun7385
left a comment
There was a problem hiding this comment.
@tallcoleman Thank you for your prompt review. I just updated the file paths, git action, the save function. I also excluded unnecessary files that you mention earlier. Feel free to have a look!
tallcoleman
left a comment
There was a problem hiding this comment.
Thanks for making those updates! Nearly there, just two blocking issues that need to be resolved:
- fix
OUTPUT_DIRand a bug you've helped us find withsave_geo_output - remove output file
src/bikespace_data/bike_thefts/source_files/bicycle-thefts_raw.geojson
There was a problem hiding this comment.
@tallcoleman As promised, the file path was fixed. Feel free to take a look. I did add a new function to build additional description. Feel free to have a look!
tallcoleman
left a comment
There was a problem hiding this comment.
Thanks for your hard work! This is now approved and ready to merge.
Please squash merge and make sure to add a descriptive message to the merge commit.
tallcoleman
left a comment
There was a problem hiding this comment.
One last thing to fix and then you're good to merge
Bike theft datasetThank you @tallcoleman for reviewing.
Merging Now! |
No description provided.