ABI-Covid-19 / moh-data

Apache License 2.0
0 stars 1 forks source link

`get_cumulative_confirmed_cases_on_day()` and the like don't take into account blank days #8

Open agarny opened 4 years ago

agarny commented 4 years ago

If we do something like:

import moh_data.main as md
data = md.Basic()
data.get_cumulative_confirmed_cases_on_day(0)

then, we get 1, as expected. However, if we then do:

data.get_cumulative_confirmed_cases_on_day(1)

we get 2 while it should still be 1 since the first confirmed case was on 26 February while the second one was on 2 March. In the same way, if we do:

data.get_cumulative_confirmed_cases_on_day(1)

we get 4 while it should still be 1.

In other words, get_cumulative_confirmed_cases_on_day() doesn't into account the fact that some days may not have had confirmed cases.

mahyar-osn commented 4 years ago

Oh yes - I didn't realize that I should leave gaps between each date that's available in the MoH data (duh!). Since the data only contains the dates for which there have been cases since Feb 26th, it does not account for the dates in between. Anyway will fix this and make a new PR.

agarny commented 4 years ago

Thanks @mahyar-osn. Otherwise, in your FindExcelFile class, you are doing:

url_reader = urlopen(self._cases_url)
try:
    html = url_reader.read().decode('utf-8')
finally:
    url_reader.close()

but this is not needed.

mahyar-osn commented 4 years ago

You're right. I missed that part after I started using BeautifulSoup! Will remove it in the new PR too.