Conversation
1) Add coverage test for function read_bm_file at line 422 of: climada/entity/exposures/litpop/nighlight.py 2) Invert the arguments of the path fucntion at line 441 of the file: climada/entity/exposures/litpop/nighlight.py. The wrong order of the arguments lead to the creation of a wrong path, which lead to an error when opening the GeoTiff file as a Gdal DataSet.
A whitespace caused a linter issue to raise
1) delate a whitespace in read_bm_file in nightlight.py 2) add test for function path() which is used in read_bm_file 3) change the way the path is created
Function tested: read_bm_file situated in: climada/entity/exposures/litpop/nightlight.py Changes: 1) Move test to integration test 2) Reduce the function arguments of read_bm_file from 2 to 1. 3) Change how exeption are raised in read_bm_file so that the error message is more explicit.
Test checks that function download_nl_files() works correctly.
|
|
||
|
|
||
| def read_bm_file(bm_path, filename): | ||
| def read_bm_file(file_path): |
There was a problem hiding this comment.
Although I agree, a single argument would be preferable here, this change affects the signature of a public function and hence we must keep backwards compatibility. Please revert this.
As a compromise one could make it flexible, i.e., allow bm_path, filename and file_path as named arguments - and possibly even *args as unnamed ones. But I think that would be out of scope of this PR.
There was a problem hiding this comment.
Hi @emanuel-schmid , I changed the arguments of the function back to where it was before. Since I had already written another test meanwhile on that branch, it is also part of the commit. If it is a problem i can revert the commit, stash the additional test and commit only the changes to read_bm_files. Thanks for the review!
1 ) Ripristine 2 orginal arguments of read_bm_file function 2) Add test for unzip_tif_to_py function of nightlight.py
| from climada.entity.exposures.litpop import nightlight | ||
| from climada.util.constants import SYSTEM_DIR | ||
| from climada.util.constants import (SYSTEM_DIR, CONFIG) | ||
|
|
||
| BM_FILENAMES = nightlight.BM_FILENAMES | ||
| DATA_DIR = CONFIG.exposures.test_data.dir() | ||
|
|
There was a problem hiding this comment.
@emanuel-schmid for the DATA_DIR ? Sorry, it was a remaining of the old test that i tried, it can be delated. Regarding the CONFIG: since CONFIG is imported from the same file as SYSTEM_DIR I thought it was more clean to import it with brackets.
| arr1 = band1.ReadAsArray() | ||
| del band1 |
There was a problem hiding this comment.
@NicolasColombi : do you know why band1 was deleted in the previous version? Why is the deletion skipped now?
(del something summons the garbage collector)
There was a problem hiding this comment.
Hi @emanuel-schmid, Yes, it is because in the old version extracting the first band of the image (band1) and then reading it as an array was done in 3 steps: 1) get the raster band , convert raster to array, delate the raster band1 in non array format. By getting the first band and directly reading it as an array (line 446) nothing is stored, so no need to delate anything. Line 446 of the new version does the same as Lines 445 to 447 of the old.
|
🙌 |
2 participants
Changes proposed in this PR:
Add coverage tests for module
nightlight.pyFunction tested:
1 )
read_bm_file2)
download_nl_files3)
unzip_tif_to_pysituated in:
climada/entity/exposures/litpop/nightlight.pyChanges:
message is more explicit.
PR Author Checklist
develop)PR Reviewer Checklist