-
Notifications
You must be signed in to change notification settings - Fork 20
[GAIA-21588] Create a function to convert v2prime result into dataframe #379
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
Conversation
This is a REST API, not a pandas endpoint.
We should support standards like CSV or JSON.
…On Wed, May 10, 2023 at 10:11 PM Muzi Gao ***@***.***> wrote:
See this discussion:
https://gro-intelligence.slack.com/archives/CUVUBHQE9/p1683578154127809?thread_ts=1683568514.990159&cid=CUVUBHQE9
I'm still debating whether or not we want to make dataframe as the default
response format for get_data_points, but now I'm making a separate function:
- converted timestamp into date (so it's human readable)
- merge series_description into combined dataframe
------------------------------
You can view, comment on, or merge this pull request online at:
#379
Commit Summary
- 255ba81
<255ba81>
update docstring
- d7e5c5b
<d7e5c5b>
new df function
- 960b8f1
<960b8f1>
add comments
File Changes
(1 file <https://github.com/gro-intelligence/api-client/pull/379/files>)
- *M* groclient/experimental.py
<https://github.com/gro-intelligence/api-client/pull/379/files#diff-a9102b652783726e7c400ed0a46b6c87f8c0e22a2d3ce666dbba882d2e94e211>
(83)
Patch Links:
- https://github.com/gro-intelligence/api-client/pull/379.patch
- https://github.com/gro-intelligence/api-client/pull/379.diff
—
Reply to this email directly, view it on GitHub
<#379>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACW4Q7GLLKSV4DMZBF6PDTXFRDEZANCNFSM6AAAAAAX5OS7V4>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
The contents of this email message and any attachments are intended solely
for the addressee(s) and may contain confidential and/or privileged
information and may be legally protected from disclosure. If you are not
the intended recipient of this message or their agent, or if this message
has been addressed to you in error, please immediately alert the sender by
reply email and then delete this message and any attachments. If you are
not the intended recipient, you are hereby notified that any use,
dissemination, copying, or storage of this message or its attachments is
strictly prohibited.
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on the output and conversion of pandas DataFrame
} | ||
}, | ||
{ | ||
"data_points": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have more than one data points?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data_points is the list of data points per series, yes you can have multiple series in response, but each series dic should only have one data_points attribute
groclient/experimental_test.py
Outdated
mock_get_data_points.return_value = mock_v2_prime_data_response.copy() | ||
df = self.client.get_data_points_df(**mock_v2_prime_data_request) | ||
|
||
self.assertEqual(df.iloc[0]["start_timestamp"], date(2023, 5, 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use pandas.testing
assert_frames_equal, and init expected df directly so that it's easier to read:
https://github.com/gro-intelligence/gro/blob/main/infrastructure/derived-compute/historical_stats/test_generate_10y_avg.py#L28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha that's a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
groclient/experimental.py
Outdated
return data_stream_list | ||
|
||
|
||
def get_data_points_df(self, **selections): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add type annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor updates requested, otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
See this discussion: https://gro-intelligence.slack.com/archives/CUVUBHQE9/p1683578154127809?thread_ts=1683568514.990159&cid=CUVUBHQE9
I'm still debating whether or not we want to make dataframe as the default response format for get_data_points, but now I'm making a separate function: