Skip to content

Feature/coverage nightlight #659

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.

Sign up for GitHub

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

Jump to bottom
Merged
emanuel-schmid merged 10 commits into from
May 2, 2023
Merged

Feature/coverage nightlight #659

emanuel-schmid merged 10 commits into from
May 2, 2023

Conversation

@NicolasColombi
Copy link
Collaborator

@NicolasColombi NicolasColombi commented Feb 18, 2023 •
edited
Loading

Changes proposed in this PR:

Add coverage tests for climada/entity/exposures/litpop/nightlight.py

For nightlight:

  1. Add tests for functions:
    - load_nasa_nl_shape_single_tile
    - load_nightlight_noaa
    - untar_noaa_stable_nighlight
    - get_required_nl_files
    - check_nl_local_file exist
  2. Add raises message if wrong layer is selected in function: load_nasa_nl_shape_single_tile
  3. Add logger message in function: load_nasa_nl_shape_single_tile
  4. Correct test: test_unzip_tif_to_py
  5. Apply uniform formatting across all tests of nightlight.py module

For litpop:

  1. Add additional tests for functions:
    - from_nightlight_intensity
    - from_population

PR Author Checklist

PR Reviewer Checklist

@NicolasColombi
Add coverage test for nightlight
c682bca 
1) Add tests for functions:
       - load_nasa_nl_shape_single_tile
       - load_nightlight_noaa
       - untar_noaa_stable_nighlight
2) Add raises message if wrong layer is selected in function:
     load_nasa_nl_shape_single_tile
3 ) Add logger message in function: load_nasa_nl_shape_single_tile
4)  Correct test unzip_tif_to_py
5)  Apply uniform formatting across all tests of nightlight.py module
@NicolasColombi
Fix pylint errors
8c4f68c 
1) line too long
2 )use %s for logging instead of f-strings
@NicolasColombi NicolasColombi marked this pull request as ready for review February 18, 2023 13:54
@NicolasColombi
Complete test for gpw_population function
8e719c4 
Add small test for function load_gpw_pop_shape() situated in:
climada/entity/exposures/litpop/gpw_population.py
@NicolasColombi
Add test for litpop functions
fbbb9ce 
Add test for functions:

1) from_nightlight_intensity()
2) from_population()
@chahank chahank requested review from carmensteinmann and samluethi and removed request for carmensteinmann March 28, 2023 08:32
@samluethi
Copy link
Collaborator

samluethi commented Mar 29, 2023

Hi @NicolasColombi,
thanks a lot for the work that went into expanding the test coverage - all format requirements etc. are met, well done!
Also, (and probably more importantly) all tests pass when I run them on my machine. Hence, again - well done =)

I still have two comments:
(1) When running the tests for the first time, I need to download a lot of data (it does this automatically). This makes the testing very slow (>5 minutes). When I re-run the test, it is a lot faster (because some files are downloaded already of course).
-> I'm not sure what the current policy is here (maybe @chahank can advise). But I think apart from the function that tests if the download is working (test_download_nl_files) no tests should download data and we should provide small test data (but really, please double check with @chahank as I'm not sure)

(2) there are still some functions that are not tested (load_nasa_nl_shape, get_required_nl_files, check_nl_local_file_exists, load_nightlight_nasa)
-> as load_nasa_nl_shape depends on get_required_nl_files and check_nl_local_file_exists you could think of testing only the latter two (as all the other dependencies in load_nasa_nl_shape are tested for)
-> for load_nightlight_nasa you could probably build a test quite quickly in line with load_nightlight_noaa which is already tested for

But again, the second point is only a suggestion, for the first please quickly check that it is fine that a test downloads data.

Thanks a lot for your work
Sam

@NicolasColombi
Copy link
Collaborator Author

NicolasColombi commented Mar 29, 2023 •
edited
Loading

Hi @NicolasColombi, thanks a lot for the work that went into expanding the test coverage - all format requirements etc. are met, well done! Also, (and probably more importantly) all tests pass when I run them on my machine. Hence, again - well done =)

I still have two comments: (1) When running the tests for the first time, I need to download a lot of data (it does this automatically). This makes the testing very slow (>5 minutes). When I re-run the test, it is a lot faster (because some files are downloaded already of course). -> I'm not sure what the current policy is here (maybe @chahank can advise). But I think apart from the function that tests if the download is working (test_download_nl_files) no tests should download data and we should provide small test data (but really, please double check with @chahank as I'm not sure)

(2) there are still some functions that are not tested (load_nasa_nl_shape, get_required_nl_files, check_nl_local_file_exists, load_nightlight_nasa) -> as load_nasa_nl_shape depends on get_required_nl_files and check_nl_local_file_exists you could think of testing only the latter two (as all the other dependencies in load_nasa_nl_shape are tested for) -> for load_nightlight_nasa you could probably build a test quite quickly in line with load_nightlight_noaa which is already tested for

But again, the second point is only a suggestion, for the first please quickly check that it is fine that a test downloads data.

Thanks a lot for your work Sam

Hi @samluethi, Thank you for the review and the comments!

Regarding point 1)

What you say is very true, and it applies to unittest (test which are runned everytime a commit is pushed). Even though we haven’t adopted a clear definition yet for unit and integration tests, the idea is that unittests shouldn't call any other climada function (except the one they are testing), should only use small mock data and should be fast.
Integration test on the other hand (runned only once a day at night, found in climada/test), are made to test the interaction between functions or modules, should therefore call other functions and are allowed to be slower (reasonably). In practice, we don't have that distinction in climada yet, all slow test are put in the integration folder, for now.
That said, you are right, >5 minutes is long... even for integration tests... I will have a look to see if I can optimize them. Thanks!

Regarding point 2)

Thanks for pointing that out, I will try to finish the tests of nightlight.py so that the class will be all tested.

I will let you know when I will be done with the changes ;)

Cheers
Nicolas

@samluethi
Copy link
Collaborator

samluethi commented Mar 30, 2023

Thanks a lot @NicolasColombi ;)

@NicolasColombi
Add nighlight test
daad87b 
Add test for function climada/entity/exposures/litpop/nightlight.py
get_required_nl_files()

remove unnecessary brackets from nightlight.py
@NicolasColombi
Merge branch 'develop' into feature/coverage_nightlight
00c8f27
@NicolasColombi
Add test for check_nl_local_file_exists
12d847f
@NicolasColombi
Merge branch 'develop' into feature/coverage_nightlight
0902bb6
@NicolasColombi
correct check_nl_local_file_exists
6379c08 
2 BM_FILES in local data folder downloaded with climada
but 5 BM_FILES on the system dir of jenkins
@NicolasColombi
correct test
2eae211
@emanuel-schmid emanuel-schmid merged commit cff2ee7 into develop May 2, 2023
@emanuel-schmid emanuel-schmid deleted the feature/coverage_nightlight branch May 2, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samluethi samluethi Awaiting requested review from samluethi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@NicolasColombi @samluethi @emanuel-schmid