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

99 select layer #167

Merged
merged 10 commits into from
Oct 17, 2023
Merged

99 select layer #167

merged 10 commits into from
Oct 17, 2023

Conversation

SergioRec
Copy link
Contributor

@SergioRec SergioRec commented Oct 5, 2023

Description

  • Added functionality to define band when loading raster file in transport_peformance.population.rasterpop.
  • Added defences to test expected behaviour of the argument.
  • Added relevant unit tests.
  • Modified docstrings.

Fixes #99

Motivation and Context

GHSL raster files only include a single band. However, if using different data sources, a functionality may be needed to select a specific band when loading the data (e.g. cases where the population data is in a band other than the first).

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?

  • Run code snippets in isolation to test the behaviour of the xarray sel method.
  • Run relevant functions with a dummy raster file with more than one band.
  • Run relevant parts of the e2e.py notebook to ensure pipeline works.
  • Run unit tests locally.

Test configuration details:

  • OS: macOS Ventura 13.6
  • Python version: 3.9.13
  • Java version: n/a
  • Python management system: conda/pip

Advice for reviewer

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

Made three small changes unrelated to the issue:

  • Small typo in test_rasterpop_get_pop_raises docstring (get_data() -> get_pop()).
  • Small typo in test_plot_folium_no_uc function name.
  • Added centre_crs argument to urban centre module call in e2e.py notebook.

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (e7c6e7f) 97.78% compared to head (287b6b6) 97.79%.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #167   +/-   ##
=======================================
  Coverage   97.78%   97.79%           
=======================================
  Files          14       14           
  Lines        1174     1179    +5     
=======================================
+ Hits         1148     1153    +5     
  Misses         26       26           
Flag Coverage Δ
unittests 97.79% <100.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
src/transport_performance/population/rasterpop.py 100.00% <100.00%> (ø)

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

Copy link
Collaborator

@ethan-moss ethan-moss left a comment

Choose a reason for hiding this comment

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

Thanks for putting these changes together - they LGTM.

I've made 2 small tweaks:

  • Swapped sel() call after clip() during file opening - selecting the band after clipping gave a better performance when reading in data, without impacting the data being read in (commit a7e3cd4).
  • Merged with dev and updated the _xds.shape size check to use _check_iter_length() now that those updates were available in dev (commits 32155a2 and 287b6b6).

I've reviewed the new feature and all seems to be working as expected. The additions/changes to the unit tests reflect well the new band selection functionalities added to the RasterPop class. All unit tests pass with no new warnings, and the coverage related to these additions is 100%.

I'm happy to merge this. It's brings the population module nicely in line with urban_centres now that a band can be selected. Thanks again!

@ethan-moss ethan-moss merged commit 6f9ed81 into dev Oct 17, 2023
@ethan-moss ethan-moss deleted the 99-select-layer branch October 17, 2023 14:34
github-actions bot pushed a commit that referenced this pull request Oct 17, 2023
* feat: added band option to _read_and_clip

* fix: removed squeeze as does not work with 2x2 xarrray

* feat: added defences for band argument

* test: remove added extra dimension from xarr_1_aoi fixture

* fix: small change to docstring in tests

* tests: added tests to check band argument behaviour

* fix: added centre_crs argument to urban centre call

* fix: clip then sel after opening

* feat: use _check_iter_length on _xds.shape

---------

Co-authored-by: Ethan Moss <[email protected]> 6f9ed81
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.

Add capability to select raster layer band in population module
4 participants