datacommonsorg / api-python

Python client library to access Data Commons
https://www.datacommons.org
Apache License 2.0
70 stars 39 forks source link

fix unsorted series in build_time_series function, change doc string … #172

Closed manojbalaji1 closed 3 years ago

manojbalaji1 commented 3 years ago

…to add sorted timeseries

fix the issue raised in #150

Issue raised: series should be sorted

Changes changes in file: datacommons_pandas/df_builder.py changes in line: L49, L53

Change Description: changed the return statement of function build_time_series by adding a suffixing .sort_index() also changed the docstring last line from " representing a time series satisfying all optional args." to "representing a sorted time series satisfying all optional args."

Test Result platform darwin -- Python 3.7.10, pytest-6.2.4, py-1.10.0, pluggy-0.13.1 rootdir: /Users/manojbalaji/personal/api-python collected 45 items

datacommons/test/core_test.py ........... [ 24%] datacommons/test/places_test.py ......... [ 44%] datacommons/test/populations_test.py ........ [ 62%] datacommons/test/query_test.py .. [ 66%] datacommons/test/set_api_key_test.py .... [ 75%] datacommons/test/stat_vars_test.py ...... [ 88%] datacommons_pandas/test/df_builder_test.py ..... [100%]

Note: Not tested for Python2 since its deprecated

manojbalaji1 commented 3 years ago

Sure, let me get on to it. Should I include it in the same PR or a separate one?

shifucun commented 3 years ago

Same PR sounds good. Thanks!

On Fri, Jun 4, 2021 at 9:58 AM Manoj Balaji J @.***> wrote:

Sure, let me get on to it. Should I include it in the same PR or a separate one?

— You are receiving this because your review was requested.

Reply to this email directly, view it on GitHub https://github.com/datacommonsorg/api-python/pull/172#issuecomment-854875056, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNNC4C52KXTGWLG4G2DDJ3TREA2XANCNFSM46C6GSRQ .

manojbalaji1 commented 3 years ago

Same PR sounds good. Thanks! On Fri, Jun 4, 2021 at 9:58 AM Manoj Balaji J @.***> wrote: Sure, let me get on to it. Should I include it in the same PR or a separate one? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#172 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNNC4C52KXTGWLG4G2DDJ3TREA2XANCNFSM46C6GSRQ .

@shifucun for testing empty data testcase, pandas.Series({}) raises the following warning,

datacommons_pandas/df_builder.py:53: DeprecationWarning: The default dtype for empty Series will be 'object' instead of 'float64' in a future version. Specify a dtype explicitly to silence this warning. observation_period, unit, scaling_factor)).sort_index()

Should I make it as

with self.assertWarns(Warning): dcpd.build_time_series('geoId/06', 'Count_Person', 'DNE') --> this returns empty series

or ignore the warning and compare it with pd.Series({}) to see if passes

manojbalaji1 commented 3 years ago

Changed code in build_time_series to handle depracation warning thrown by pd.Series({}) Added unittest for function build_time_series in https://github.com/datacommonsorg/api-python/blob/master/datacommons_pandas/test/df_builder_test.py

Output for ./run_tests_local.sh

============================================================================================================ test session starts ============================================================================================================= platform darwin -- Python 3.7.10, pytest-6.2.4, py-1.10.0, pluggy-0.13.1 rootdir: /Users/manojbalaji/personal/api-python collected 48 items

datacommons/test/core_test.py ........... [ 22%] datacommons/test/places_test.py ......... [ 41%] datacommons/test/populations_test.py ........ [ 58%] datacommons/test/query_test.py .. [ 62%] datacommons/test/set_api_key_test.py .... [ 70%] datacommons/test/stat_vars_test.py ...... [ 83%] datacommons_pandas/test/df_builder_test.py ........ [100%]

============================================================================================================= 48 passed in 1.65s =============================================================================================================