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.

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

160 148 defences #177

Merged
merged 22 commits into from
Oct 17, 2023
Merged

160 148 defences #177

merged 22 commits into from
Oct 17, 2023

Conversation

SergioRec
Copy link
Contributor

Description

  • Modified _check_list and _check_item_in_list to accept any iterable. Renamed to _check_iterable and _check_item_in_iter.
  • Added functionality to check length of iterable.
  • Modified defences in urban centres module to use defence.py functions whenever possible.
  • Modified references to old functions throughout code.
  • Added tests for modified functions, testing new functionality. Modified tests for defences to check for new raise statements.

Fixes #148
Fixes #160

Motivation and Context

Motivation for these changes is try to use consistent defences, and to turn the most common defences into functions to reuse.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Tests run without fail.
  • Manually checked behaviour of modified functions.
  • Notebooks run without fail.

Test configuration details:

  • OS: MacOS Ventura 13.6
  • Python version: 3.9.16
  • Java version: n/a
  • Python management system: pip/conda

Advice for reviewer

I haven't checked to replace any defence in other modules other than urban centres, as this would make the dif too big and messy. Better to add these as new tickets.

Checklist:

  • My code follows the intended structure of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Additional comments

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (fa03811) 97.72% compared to head (e63508b) 97.71%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #177      +/-   ##
==========================================
- Coverage   97.72%   97.71%   -0.02%     
==========================================
  Files          12       12              
  Lines        1012     1005       -7     
==========================================
- Hits          989      982       -7     
  Misses         23       23              
Flag Coverage Δ
unittests 97.71% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/transport_performance/gtfs/gtfs_utils.py 100.00% <100.00%> (ø)
src/transport_performance/gtfs/validation.py 96.80% <100.00%> (ø)
src/transport_performance/osm/osm_utils.py 77.14% <100.00%> (ø)
...c/transport_performance/urban_centres/raster_uc.py 100.00% <100.00%> (ø)
src/transport_performance/utils/defence.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CBROWN-ONS CBROWN-ONS self-assigned this Oct 12, 2023
Copy link
Contributor

@CBROWN-ONS CBROWN-ONS left a comment

Choose a reason for hiding this comment

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

Overview

Hi @SergioRec.
Great job getting all of this done, all features function well and as expected. All features are also tested comprehensively to a high standard.

Suggested Changes

Along with the comments added to this review, I also have more changes that didn't associate to lines touched within this PR (however are related to the PR).

  • Lines 358-362 in raster_uc.py: Using the _check_iter_length() defence function would be useful here.

@CBROWN-ONS
Copy link
Contributor

HI @SergioRec . I've looked through the changes and all the requested changes seem to be acted upon. Great job getting these done. Will merge into dev now.

@CBROWN-ONS CBROWN-ONS merged commit e7c6e7f into dev Oct 17, 2023
github-actions bot pushed a commit that referenced this pull request Oct 17, 2023
* refactor: changed type defences to module

* tests: updated match error messages

* feat: replaced list functions to accept any iterable

* chore: replace function names in code

* refactor: changed tuple defence in uc

* fix: replaced import to prevent deprecation warning

* fix: typo in imported function

* fix: fixed function argument names and gtfs tests

* tests: changes to defence tests

* tests: finished changes to defence tests

* feat: added check iterable lenght func

* refactor: change centre tuple length check in uc

* tests: add tests for iterable length

* fix: minor corrections to match error messages

* Reorganise imports; Update comment specifying PosixPath to Path

* add space for pre-commit;

* Best practice changes;Update type hinting and associated docstring

* chore: run pre-commit

* style: changes to docs, style and defences as suggested in review

* fix: changed defences in cleaners.py

* fix: fixed check for type None

---------

Co-authored-by: Charlie Browning <[email protected]>
Co-authored-by: Ethan Moss <[email protected]> e7c6e7f
@CBROWN-ONS CBROWN-ONS deleted the 160-148-defences branch October 20, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the urban centres module to utilise defence.py Generalise _check_list() to check any iterable
4 participants