Add get_pop_obs api #93

shifucun · 2019-08-17T22:32:45Z

No description provided.

antaresc

Thanks for implementing this! I added a number of comments about documentation and placement of code.

Can you also update the .rst files in /docs/ and make sure the docstrings render locally in the sphinx build?

datacommons/places.py

antaresc · 2019-08-18T17:44:17Z

datacommons/places.py

  return result
+
+
+def get_pop_obs(dcid):


Should we move this to the Populations module? The language of get_pop_obs documentation matches more get_populations and get_observations, than it does get_places_in.

Later on will add get_place_obs, get_place_kml, similar to this one, can be thought of "properties" or "nodes" about a place. So in the places module, a user can find all apis to get information about a place. Also get_pop_obs can return observation directly linked to a place (I know the function name is not accurate then), where no population is present at all.

Ah okay that's fine. This raises a couple questions for me actually.

I think the distinction between get_place_obs and get_pop_obs is a bit confusing now. I would have assumed that observations linked directly to place would be returned by get_place_obs.

Also how is the structure of the return value if the observation is linked directly to the place dcid (in particular, how do you differentiate Observations linked by "location" vs. "childhoodLocation"?

Is it possible to separate get_pop_obs into two functions with one returning population observation pairs, and the other returning observations linked directly to the Place?

I'm also wondering if it all needs to be returned in one call. As is, the call seems very browser specific. Also, are these calls place specific? If I understand correctly, we could have statistical population on non-place nodes.

Would we run into limit issues with this call? Or is everything going to be in the cache.

Ah okay that's fine. This raises a couple questions for me actually.

I think the distinction between get_place_obs and get_pop_obs is a bit confusing now. I would have assumed that observations linked directly to place would be returned by get_place_obs.
They do have things in common, kinda like get_triples and get_property_values. get_place_obs get observations for a particular population spec, while get_pop_obs get everything. We need to inform users from the comments.

Also how is the structure of the return value if the observation is linked directly to the place dcid (in particular, how do you differentiate Observations linked by "location" vs. "childhoodLocation"?
There is an "observations" field to contain all those. Currently there is no way to tell that, this is a bug.

Is it possible to separate get_pop_obs into two functions with one returning population observation pairs, and the other returning observations linked directly to the Place?

Might be easier to put them in one, this is like get_triples to get everything. get_place_obs is like get_prop_values.

This is only from cache, so limit is not an issue.

get_place_obs gives observation of places, with the population specification fixed.

get_pop_obs actually or (ultimately) get observations for a place, i would see the population here is a by-product. This is like get city in USA, the county is just a by-product.

If this could return non-place results, should this move to the populations module? Unless the mixer currently only returns place results. Seeing the layout of functions across modules now, I think this belongs with populations.. sorry if I misunderstood your comments.

Yea, moved to populations module

antaresc · 2019-08-18T17:47:44Z

datacommons/places.py

+              'marginOfError': 119,
+              'measuredProp': 'count',
+              'measuredValue': 225,
+              'measurementMethod': 'CenusACS5yrSurvey',


Can we choose BLS as the example population? We should try to avoid any observation that has measurementMethod "Cenus"ACS5YrSurvey until that's fixed in the graph.

Done. Good point!

Ah it's back to CenusACS5yrSurvey?

um, I made it Census now..

datacommons/places.py

antaresc · 2019-08-18T17:51:41Z

datacommons/examples/places.py

  pd_frame = dc.flatten_frame(pd_frame)
  print(pd_frame)

+  # Get all population and observation data of Mountain View.


Maybe move to datacommons/examples/populations.py. See comment above about moving get_pop_obs to populations submodule.

See reply on the other comment.

shifucun

Thanks for comments on the docstring!

shifucun · 2019-08-18T20:42:48Z

datacommons/places.py

  return result
+
+
+def get_pop_obs(dcid):


Later on will add get_place_obs, get_place_kml, similar to this one, can be thought of "properties" or "nodes" about a place. So in the places module, a user can find all apis to get information about a place. Also get_pop_obs can return observation directly linked to a place (I know the function name is not accurate then), where no population is present at all.

shifucun · 2019-08-18T20:43:19Z

datacommons/examples/places.py

  pd_frame = dc.flatten_frame(pd_frame)
  print(pd_frame)

+  # Get all population and observation data of Mountain View.


See reply on the other comment.

datacommons/places.py

shifucun · 2019-08-18T20:58:20Z

datacommons/places.py

+              'marginOfError': 119,
+              'measuredProp': 'count',
+              'measuredValue': 225,
+              'measurementMethod': 'CenusACS5yrSurvey',


Done. Good point!

datacommons/places.py

antaresc

Left a bunch of nits, but otherwise looks good!

PTAL at my questions from a comment above before submitting.

datacommons/places.py

beets · 2019-08-19T18:49:11Z

datacommons/places.py

  return result
+
+
+def get_pop_obs(dcid):


I'm also wondering if it all needs to be returned in one call. As is, the call seems very browser specific. Also, are these calls place specific? If I understand correctly, we could have statistical population on non-place nodes.

Would we run into limit issues with this call? Or is everything going to be in the cache.

datacommons/utils.py

datacommons/places.py

antaresc · 2019-08-19T20:58:47Z

datacommons/places.py

+              'marginOfError': 119,
+              'measuredProp': 'count',
+              'measuredValue': 225,
+              'measurementMethod': 'CenusACS5yrSurvey',


Ah it's back to CenusACS5yrSurvey?

* Fix typo in places.py. (#71) (#84) * Added iPynb notebooks and fixed issue #67 (#86) * Added notebooks * Added --upgrade flag * Update requirements and setup.py * Added maintenance information to the readme * Update setup.py and pypi long description (#87) * Update setup.py and requirements * Add long description for pypi package * Use README.md as the long description * Use single quote * Fixed docs and updated changelog (#89) * Amended documentation for creating a key. * Updated CHANGELOG * Spacing * Added 0.4.3 changes * Added 0.x to changelog * Add get_pop_obs api (#93) * Add get_pop_obs api * Updated comments with more detailed description. * Update comment and rst file * Updated comments * Move to populations module * Add missing change * Correct typo * More typo correction * Add comments on observations field * Add the step to join google group (#95) * Add more details for joining google group (#96) * Implemented get_place_obs and reimplemented query (#94) * Implemented get_place_obs * Reimplemented query as a function instead of a class. * query now returns a list. Amended docstrings. * Fixed docstring typo * Add observation_date as argument to get_place_obs (#97) * Add observation_date as argument to get_place_obs * Update doc string * Update test * fix link to getting started on read the docs home page (#98) * Fixed bug regarding indexing (#100) * Updated the changelog (#101)

Add get_pop_obs api

96cbfbb

shifucun requested review from Spaceenter and antaresc August 17, 2019 22:32

Spaceenter approved these changes Aug 18, 2019

View reviewed changes

antaresc suggested changes Aug 18, 2019

View reviewed changes

antaresc requested a review from beets August 18, 2019 17:53

Updated comments with more detailed description.

4e07a2d

shifucun commented Aug 18, 2019

View reviewed changes

Update comment and rst file

bf88ed9

antaresc approved these changes Aug 19, 2019

View reviewed changes

datacommons/places.py Outdated Show resolved Hide resolved

datacommons/places.py Outdated Show resolved Hide resolved

datacommons/places.py Outdated Show resolved Hide resolved

datacommons/places.py Outdated Show resolved Hide resolved

Spaceenter approved these changes Aug 19, 2019

View reviewed changes

beets reviewed Aug 19, 2019

View reviewed changes

Bo Xu added 3 commits August 19, 2019 12:03

Updated comments

ff70a1a

Move to populations module

ca6a08a

Add missing change

8365dde

antaresc suggested changes Aug 19, 2019

View reviewed changes

Bo Xu added 2 commits August 19, 2019 14:02

Correct typo

c48959d

More typo correction

6a09413

beets approved these changes Aug 19, 2019

View reviewed changes

Add comments on observations field

30ecd7b

shifucun requested a review from antaresc August 20, 2019 18:08

antaresc approved these changes Aug 20, 2019

View reviewed changes

shifucun merged commit 5cf41f7 into datacommonsorg:master Aug 20, 2019

shifucun mentioned this pull request Aug 30, 2019

[API v1.0.0 Feedback] Implement GetPopObs and GetPlaceObs wrappers. #92

Closed

Add get_pop_obs api #93

Add get_pop_obs api #93

Uh oh!

Conversation

shifucun commented Aug 17, 2019

Uh oh!

antaresc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antaresc Aug 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shifucun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

antaresc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

antaresc Aug 18, 2019 •

edited

Loading