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

351 add highly serviced stops scot #360

Closed jwestw closed 1 year ago

jwestw commented 1 year ago

Order to merge pull requests: 1) This one: #360 2) #361 3) #366

Pull Request submission

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

Nothing extra needed in terms of dependencies

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

Make sure you have all the Scotland data from sync! It won't run without those

Closes or fixes

Closes #351

Code

Documentation

Any new code includes all the following forms of documentation:

Data

Testing

james-westwood commented 1 year ago

I just rebased master to this branch to kill of the merge conflicts. https://github.com/ONSdigital/SDG_11.2.1/compare/005a81d62c7e213bdc9e069b7d2fd2ff2e2282db..497e18b0fabc0a0823b077ed4b1b9abefe32b985 Should be all ready to go now.

paigeh-fsa commented 1 year ago

When import from main in Scotland, then will run the whole script. Therefore, need to add if name = main block.

paigeh-fsa commented 1 year ago

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.

jwestw commented 1 year ago

@nkshaw23 I have fixed the problem of importing the df from main now. the `if name == 'main'" block was in the wrong place.

jwestw commented 1 year ago

The code now runs into another problem (at least for me). This is unrelated to my ticket and is detailed here: https://github.com/ONSdigital/SDG_11.2.1/issues/367

nkshaw23 commented 1 year ago

Code ran through with the following output - seems slightly suspicious to me that every person is served but need to look in QGIS to see whats going on. This happened for another LA aswell so need to investigate.

image

A few general comments for the scotland process (not necessarily related to the MR code)

1) The oa to la mapper is massive. It contains mappings for the whole of the UK and we only use 2 columns. Could strip the dataset and re-upload to sync?

2) A couple of warnings of mixed dytpes

image image

Antonio-John commented 1 year ago

The pipeline is running successfully however there is still a need to QA the figures this is covered in ticket #352 A ticket about resolving the warnings has been created here #370