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

Making multi-level indexing optional #26

Merged
merged 2 commits into from
Oct 17, 2022
Merged

Making multi-level indexing optional #26

merged 2 commits into from
Oct 17, 2022

Conversation

cjbas22
Copy link

@cjbas22 cjbas22 commented Oct 4, 2022

This modification addresses issue #25 by adding an additional parameter multi_index to some functions. When multi_index=False the output will be a dataframe with a single-level index (datetime) independently of the number of sites being queried.

This modification addresses issue #25  by adding an additional parameter multi_index to some functions. When multi_index=False the output will be a dataframe with a single-level index (datetime) independently of the number of sites being queried.
@sblack-usu sblack-usu self-requested a review October 5, 2022 18:11
@@ -24,9 +24,12 @@
WATERDATA_SERVICES = ['qwdata', 'measurements', 'peaks', 'pmcodes', 'water_use', 'ratings']


def format_response(df, service=None):
def format_response(df, multi_index=None, service=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter order for functions matter, so it is often best to put new parameters last.

Copy link
Author

@cjbas22 cjbas22 Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, service is only used in 1 function, so it is more convinient to add it as second. If third, every call to multi_index will need to be named as service=None and it's not included most of the times. Do you think it is still better to add multi_index last?

"""Setup index for response from query.
"""
if multi_index:
multi_index=None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines guarantee multi_Index will be None, it doesn't matter what value.

dataretrieval/nwis.py Outdated Show resolved Hide resolved
@@ -71,6 +75,8 @@ def get_qwdata(datetime_index=True, wide_format=True, sites=None, start=None, en
If the qwdata parameter begin_date is supplied, it will overwrite the start parameter
end: string
If the qwdata parameter end_date is supplied, it will overwrite the end parameter
multi_index: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, multi_index is a boolean. Set the default to a boolean instead of None. That'll clean up the code in the format_response function above.

@@ -155,7 +163,7 @@ def _discharge_measurements(**kwargs):
return _read_rdb(response.text), _set_metadata(response, **kwargs)


def get_discharge_peaks(sites=None, start=None, end=None, **kwargs):
def get_discharge_peaks(sites=None, start=None, end=None, multi_index=None, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can clean up a lot of this by updating def format_response to accept kwargs and then pop multiindex only there instead of popping it higher up and cascading through functions that only pass it along.

@cjbas22 cjbas22 merged commit cbb5798 into master Oct 17, 2022
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.

2 participants