New get stat functions#142
Conversation
| from __future__ import print_function | ||
|
|
||
| import sys | ||
| sys.path.append('../') |
There was a problem hiding this comment.
not sure if this is best practice. Can you check see how people think. I remember we used to run, in the top level folder, python -m datacommons.examples.xxx
There was a problem hiding this comment.
Ok, removed and added that to readme.
|
|
||
|
|
||
| def main(): | ||
| # Dcid for Santa Clara County. |
There was a problem hiding this comment.
can you make the entire example parameterized:
data = [{place: xx, stat_var = xxx, xx}, {},{}]
for d in data:
print(xx)
print(dc.get())
| unit (:obj:`str`): Optional, the dcid of the preferred `unit` value. | ||
| scaling_factor (:obj:`int`): Optional, the preferred `scalingFactor` value. | ||
| Returns: | ||
| A :obj:`double` the value of :code:`stat_var` for :code:`place`, filtered |
There was a problem hiding this comment.
in python it should be "float" instead of double
…le pain points: mock, urllib, unicode, and base64 changes between Py2&3.
| }, | ||
| { | ||
| 'place': | ||
| 'nuts/HU22', |
There was a problem hiding this comment.
no need to newline here and below?
There was a problem hiding this comment.
This is from yapf formatting, will keep since we plan to enforce google pylint later.
|
|
||
|
|
||
| def main(): | ||
| data = [ |
There was a problem hiding this comment.
consider naming this something more specific -- params, etc?
|
|
||
| for d in data: | ||
| print('\n>>> get_stat_value: ', | ||
| [param for param in d.values() if param]) |
There was a problem hiding this comment.
is the "if param" required? i think it'll be nice to format this as:
get_stat_value(place=geoid/foo, stat_var=statVarFoo)
<<< ...
| @@ -0,0 +1,99 @@ | |||
| # Copyright 2020 Google Inc. | |||
There was a problem hiding this comment.
nit: should this file name be "get_stat.py"?
There was a problem hiding this comment.
Maybe discuss later? If we do that, probably should do that in a future PR, since in py lib, the file names are all like
It's the functions that are like "get_stat*"
| @@ -0,0 +1,147 @@ | |||
| # Copyright 2020 Google Inc. | |||
There was a problem hiding this comment.
nit: ditto re: filename. maybe statvars or stat_vars or stats to mirror the other module names?
There was a problem hiding this comment.
Ok, I can make it plural
| import datacommons.utils as utils | ||
|
|
||
|
|
||
| def _send_get_stat_req(url): |
There was a problem hiding this comment.
this seems mostly identical with _send_request -- does that not work for this module?
There was a problem hiding this comment.
_send_request is expecting a payload keyword, but the REST API does not have that for /stat. It's an open item for discussion.
There was a problem hiding this comment.
i wouldn't fork just for that reason. can we add support for non-payload requests?
There was a problem hiding this comment.
1-- discussed with Bo, he wants to move away from payload
2-- discussed with Bo, decided to add a use_payload arg to _send_request, so the code now reuses the function.
3 participants
No description provided.