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

Viirs proximity #18

Merged
merged 12 commits into from
Dec 4, 2020
Merged

Viirs proximity #18

merged 12 commits into from
Dec 4, 2020

Conversation

chloeskt
Copy link
Contributor

@chloeskt chloeskt commented Dec 4, 2020

This PR aims at:

  • adding NASA FIRMS VIIRS dataset and class to clean it
  • use a way more efficient algorithm to merge datasets by proximity (thanks to KDTree). This allows also to merge ERA5/VIIRS with FWI, to get, for 2019, around 35000 lines of data (one line = one point on one day)
  • update unittests accordingly
  • remove the TODOs left by @dataJSA (I checked with him, they should have been removed)

As always, all reviews are welcome 😄

@chloeskt chloeskt added this to the 0.1.0 milestone Dec 4, 2020
@chloeskt chloeskt self-assigned this Dec 4, 2020
@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #18 (4e3187d) into master (f65d4f3) will increase coverage by 2.35%.
The diff coverage is 98.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   94.30%   96.65%   +2.35%     
==========================================
  Files          11       11              
  Lines         351      419      +68     
==========================================
+ Hits          331      405      +74     
+ Misses         20       14       -6     
Flag Coverage Δ
unittests 96.65% <98.61%> (+2.35%) ⬆️

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

Impacted Files Coverage Δ
pyro_risks/datasets/nasa_wildfires.py 97.43% <97.36%> (+16.48%) ⬆️
pyro_risks/config.py 100.00% <100.00%> (ø)
pyro_risks/datasets/datasets_mergers.py 100.00% <100.00%> (ø)
pyro_risks/datasets/utils.py 96.99% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fb6a38...4e3187d. Read the comment docs.

Copy link
Collaborator

@jsakv jsakv 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 the PR @chloeskt, it is awesome !
There is no unittest for pyro_risks/models/xgb_classifier.py and some cases that are not covered since you have updated the pyro_risks/datasets/nasa_wildfires.py module

@chloeskt
Copy link
Contributor Author

chloeskt commented Dec 4, 2020

Thanks for the PR @chloeskt, it is awesome !
There is no unittest for pyro_risks/models/xgb_classifier.py and some cases that are not covered since you have updated the pyro_risks/datasets/nasa_wildfires.py module

Hi @dataJSA, I deleted all models file since it was not relevant for this PR (my bad). For the tests, I guess we would need to add test data in the data release to test the cases that are missed since they related to different files format if you look at the details 😄

@chloeskt
Copy link
Contributor Author

chloeskt commented Dec 4, 2020

Hi @dataJSA I've added in the release mock test files which allow to write new tests for various file formats (csv, json and xlsx). Now the coverage is higher 😄

@jsakv
Copy link
Collaborator

jsakv commented Dec 4, 2020

Hi @dataJSA I've added in the release mock test files which allow to write new tests for various file formats (csv, json and xlsx). Now the coverage is higher 😄

Thanks for your PR @chloeskt !

@jsakv jsakv self-requested a review December 4, 2020 22:42
@chloeskt chloeskt merged commit 4d010f1 into master Dec 4, 2020
@chloeskt chloeskt deleted the viirs_proximity branch December 4, 2020 23:02
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.

2 participants