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

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

Merged

Conversation

manojbalaji1
Copy link
Contributor

…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

@shifucun shifucun self-requested a review June 4, 2021 16:21
Copy link
Contributor

@shifucun shifucun 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 making the change!

As this function is updated, can you add a test which can use mock request similar to https://github.com/datacommonsorg/api-python/blob/master/datacommons_pandas/test/df_builder_test.py#L114

Let me know if you need more information

@manojbalaji1
Copy link
Contributor Author

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

@shifucun
Copy link
Contributor

shifucun commented Jun 4, 2021 via email

@manojbalaji1
Copy link
Contributor Author

manojbalaji1 commented Jun 5, 2021

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
Copy link
Contributor Author

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 =============================================================================================================

Copy link
Contributor

@shifucun shifucun 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 adding the test, it looks good!

@shifucun shifucun merged commit 16fabfc into datacommonsorg:master Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants