ONSdigital / SDG_11.2.1

Analysis for the UN Sustainable Development Goal 11.2.1
https://onsdigital.github.io/SDG_11.2.1/
Apache License 2.0
5 stars 7 forks source link

334 combine highlyservd bus train #350

Closed james-westwood closed 1 year ago

james-westwood commented 1 year ago

Pull Request submission

Insert detailed bullet points about your changes here!

Insert any instructions to help the reviewer, e.g. "install new requirements from requirements.txt"

*Let the reviewer know what data files are needed (to be grabbed from sync)

Closes or fixes

Fixes #

Code

Documentation

Any new code includes all the following forms of documentation:

Data

Testing


Peer Review Section

Final approval (post-review)

The author has responded to my review and made changes to my satisfaction.


Review comments

Insert detailed comments here!

These might include, but not exclusively:

Your suggestions should be tailored to the code that you are reviewing. Be critical and clear, but not mean. Ask questions and set actions.

james-westwood commented 1 year ago

@nkshaw23 I have created a draft PR to collect all the commits together and work from here. Might be best to keep comments about the work in the issue #334 though.

nkshaw23 commented 1 year ago

Tested for England, results viewed in QGIS. Everything looks good but would like to investigate a bit further.

@james-westwood outstanding task for this PR:

1) Build in the process for Scotland script. In an ideal world we would have these combined into one process, but we can look at this afterwards. 2) Once in scotland, can remove two functions from data_transform (add_stop_capacity_type and filter_stops) 3) Check for other functions that are no longer used

jwestw commented 1 year ago

@nkshaw23 I think it would be best to split out the Scotland stuff into it's own ticket + PR.

nkshaw23 commented 1 year ago

@james-westwood This is now ready for review as we are splitting Scotland work into another ticket (#351 )

Closes #330 Closes #334 Closes #335

nkshaw23 commented 1 year ago

Pipeline works from start to finish now. Just two comments from me.

Because we cant join NAPTAN onto the train data, we couldnt use the existing capacity_type function which relied on StopType codes from NAPTAN. Hence, the high/low capacity type has been added manually based on whether stop is bus, train or tram.

Further work on QA-ing outputs will be undertaken in #352

jwestw commented 1 year ago

I will pull the code and run it today.