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

340 change age bands in scotland #362

Closed paigeh-fsa closed 1 year ago

paigeh-fsa commented 1 year ago

Pull Request submission

I have created an if statement in data_transform.py which handles any instances where the bins are larger than 90+. It should be generalised to work with any time that there are more bins over 90. I also changed the renaming of columns in scotland.py.

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.

Antonio-John commented 1 year ago

I will review :)

Antonio-John commented 1 year ago

Review comments Insert detailed comments here!

The age_bin list needs updating in line 249. Currently it has all the old age bins. So, when these are being read into the served_proportion_disag function it doesn't recognise some of the age bins.

It would be better to make the function age_bin to return something. This is the function contained within data_ingest in bin_pop_ages.

These might include, but not exclusively:

bugs that need fixing (does it work as expected? and does it work with other code that it is likely to interact with?) alternative methods (could it be written more efficiently or with more clarity?) documentation improvements (does the documentation reflect how the code actually works?) additional tests that should be implemented (do the tests effectively assure that it works correctly?) code style improvements (could the code be written more clearly?) Your suggestions should be tailored to the code that you are reviewing. Be critical and clear, but not mean. Ask questions and set actions.

Antonio-John commented 1 year ago

The function _age_bin is correctly defined, however when it's being used it needs to be assigned a variable.

paigeh-fsa commented 1 year ago

@Antonio-John I think I have now addressed all your comments, let me know if there is anything else :)