First crack at changing the url for one function #152

ehinman · 2024-08-16T23:00:48Z

Created a new wqx3_url that is used if legacy=False, which is the default. I also added dataProfile as a kwarg (for this function at least, may not be needed for others). Thoughts on this approach? R dataRetrieval has a service argument where the user specifies if they want the old or the new profiles by name: https://github.com/DOI-USGS/dataRetrieval/blob/main/R/readWQPdata.R, but the functions in python do not let the user pick the service, and are instead specific to them (e.g. Results, Activity, Station, etc.).

Also removed zip since it's not a part of the new WQX3 url, but perhaps should leave it in for the legacy profiles and ensure it is switched to 'no'.

thodson-usgs

Yup, that seems reasonable

thodson-usgs · 2024-08-17T04:24:20Z

dataretrieval/wqp.py

+
+    else:
+        url = wqx3_url('Result')
+        if 'dataProfile' not in kwargs:


Should check whether dataProfile is among a list of valid profiles and throw an error if not. Examples in nwis.

dataretrieval/wqp.py

ehinman · 2024-08-20T23:17:34Z

Well ok, this might be worse before it gets better. I don't really want to introduce a new function input just for dealing with legacy services. I think as long as the dataProfiles remain distinct between legacy and WQX3, users can specify which URL they want to grab from (and technically they should be adapting their workflows to the new format). However, what_sites is tricky because in this iteration, I've basically removed users' ability to get the legacy "Station" service. What's more, some of the other functions only access the legacy services at this point in time.

I would like to create a nice table in the documentation to show how users can grab different legacy and WQX3 profiles.

I'll also look into a column name crosswalk...I think Laura might have one.

Thoughts on all this? Eventually, we'd hopefully be able to simply remove all of the legacy kwarg options, but keep the functions essentially the same.

thodson-usgs · 2024-08-21T13:30:49Z

I'll also look into a column name crosswalk...I think Laura might have one.

Might be unnecessary? wqp returns the raw column names and I don't think you want to crosswalk back to legacy, right?

I might try something like:

create a new function for calling legacy, which issues a deprecation warning.
within the standard function, set the default as legacy=True, that determines whether or not to call the legacy or the current profiles.
eventually, add some functions to validate whether the user is passing valid params for a given profile.
once the migration of all services is complete, set legacy=False, thereby changing the default behavior, at which point we would issue a major release.

ehinman · 2024-08-21T15:04:17Z

I like the idea of having wqp_url or a wrapper for it producing a warning. I'll add that in.

I'm trying to avoid adding a new input legacy to all of these functions (since someday it'll be useless), but rather leveraging the kwarg dataProfile, which can change with time but will hopefully remain a steady input. As long as a dataProfile name within a service is unique to legacy/wqx3, I think this will work....but see this table I made:

dr-py-ideas.xlsx

Regarding the column names, I'm not sure I follow. I'm thinking about users who have to change their coding workflow and want to see the new column names associated with WQX3, and how they map onto the legacy names. Showing them how to leverage this csv might be sufficient: https://www.epa.gov/system/files/other-files/2024-07/schema_outbound_wqx3.0.csv

ehinman · 2024-08-21T21:41:28Z

OK, after talking to @lstanish-usgs, I have added a new legacy input to the wqp functions. For a majority of functions, the only profile available is legacy right now. I need to add some new tests, but figured I'd get some reviews on the structure, first. After this PR is merged, we can then focus on a new function for the Samples Data API.

ehinman · 2024-08-30T16:46:38Z

@jzemmels Thanks for reviewing! A few things to check in your review:

Is the documentation clear on how the updated functions work? What else would be helpful to add?
Please try running these functions, pulling both legacy and wqx3 datasets using kwargs you can find in the web services guide. Do you run into any errors? Please note the error message.
Are the warning messages clear/sufficient on "stale" data pulls?

Let me know if you have additional questions. I would say bullet #2 is most important to review most thoroughly.

thodson-usgs · 2024-08-30T17:26:17Z

OK, after talking to @lstanish-usgs, I have added a new legacy input to the wqp functions.

Sounds good. Recommend setting legacy=True for now with deprecation warning. Next major release we will set to False or else create new functions.

jzemmels · 2024-08-30T22:44:37Z

Is the documentation clear on how the updated functions work? What else would be helpful to add?

I'm satisfied with the documentation updates to indicate the new legacy parameter. When a user incorrectly specifies an argument like dataProfile that must be from a small list of options, it'd be great to print the list the user must choose from in the error message. For example, "dataProfile is not a legacy profile name. Select from ['resultPhysChem', 'biological', 'narrowResult']

Also, I'm not sure if I missed this, but a short definition of each dataProfile would be helpful in the package's documentation. The definitions are a bit tricky to find in the waterqualitydata.us documentation.

jzemmels · 2024-08-30T22:49:37Z

Are the warning messages clear/sufficient on "stale" data pulls?

I think the message could start from the second sentence. The new legacy parameter implicitly communicates that some change occurred, so I don't think mentioning a change in the warning message is necessary.

Perhaps the warning message could also provide an alternative for users who want new data. E.g., "To fetch new data, set legacy = False and dataProfile to one of the following: ['fullPhysChem', 'narrow', 'basicPhysChem']"

Other than those two things, I think the warning message is good.

(Edit) Oh, one other thing: is there some way to automatically wrap the message so that it doesn't appear on one long line? Maybe this is just a VSCode thing...

jzemmels · 2024-08-30T22:59:54Z

Please try running these functions, pulling both legacy and wqx3 datasets using kwargs you can find in the web services guide. Do you run into any errors? Please note the error message.

I've checked a couple requests against what the WQP website returns and they work. I'm running into an issue where some requests that run quickly on the WQP website take a really long time to run in Python. Not sure what's causing it. I'm not done checking different combinations of kwargs, but I can check more once I return on Sept. 9.

thodson-usgs · 2024-09-03T21:43:04Z

@jzemmels, near the top of the page, you should see a big green button labeled "Add your review". I recommend you use that process to create your review and suggest changes, rather than adding comments in the issue.

Also,

a short definition of each dataProfile would be helpful in the package's documentation. The definitions are a bit tricky to find in the waterqualitydata.us documentation.

Keep the function documentation concise. The doc should note that a full description of the profiles can be found at https://www.waterqualitydata.us/beta/portal_userguide/#data-profiles. The doc should also provide a list of valid profiles, either by writing them out or by referring the user to a convenience function.

...but let's continue this discussion using the normal review process.

ehinman · 2024-09-04T18:07:25Z

Are the warning messages clear/sufficient on "stale" data pulls?

I think the message could start from the second sentence. The new legacy parameter implicitly communicates that some change occurred, so I don't think mentioning a change in the warning message is necessary.

Perhaps the warning message could also provide an alternative for users who want new data. E.g., "To fetch new data, set legacy = False and dataProfile to one of the following: ['fullPhysChem', 'narrow', 'basicPhysChem']"

Other than those two things, I think the warning message is good.

(Edit) Oh, one other thing: is there some way to automatically wrap the message so that it doesn't appear on one long line? Maybe this is just a VSCode thing...

Thanks for these suggestions. I think I'm going to keep the warning as-is for the legacy services (because they're not specific to the Result service), but I added another sentence about setting legacy=False to get the latest profiles, if available.

jzemmels

I've now tested out each function with multiple argument combinations and am pleased that nothing threw unexpected errors. The comments I've left are general usability/documentationcomments that can either be addressed here or filed under future considerations.

jzemmels · 2024-09-09T23:55:10Z

dataretrieval/wqp.py

+    if legacy is True:
+        if 'dataProfile' in kwargs:
+            if kwargs['dataProfile'] not in result_profiles_legacy:
+                raise TypeError(f"""


Nice addition!

jzemmels · 2024-09-10T00:40:31Z

dataretrieval/wqp.py

-    zip: string
-        Parameter to stream compressed data, if 'yes', or uncompressed data
-        if 'no'. Default is 'no'.
+        be 'geojson' 


GeoJSON is not yet supported, so this documentation should reflect that mimeType will automatically be changed to csv.

On a related note, is there a reason why the other possible WQP file outputs (tsv, xlsx) aren't supported here? I tested out passing mimeType="tsv" and "xlsx" to the query function and it returned the expected output. Seems like it would be a simple addition to expand the functionality.

That's a good point. Legacy supports tsv and xlsx, but it looks like beta does not yet support anything other than csv. I might add this as an issue to follow up on, but leave as-is for this PR. #162

jzemmels · 2024-09-10T00:46:16Z

dataretrieval/wqp.py

+                                Please choose from "fullPhysChem", "narrow", or "basicPhysChem"
+                                """)
+        else:
+            kwargs['dataProfile'] = 'fullPhysChem'


Genuine question: is this a reasonable default for dataProfile -- would the "typical" user want this to be set for them or should the function throw an error if the user doesn't set the dataProfile argument?

Follow whatever @ldecicco-USGS did

R dataRetrieval uses the fullPhysChem profile. That is generally based on what the EPA TADA package uses and what provides the most comprehensive amount of USGS metadata.

jzemmels · 2024-09-10T17:16:10Z

dataretrieval/wqp.py

+    if legacy is True:
+        url = wqp_url('Organization')
+    else:
+        print('No WQX3.0 profile currently available, returning legacy profile.')


I think this should throw an error if the user requests WQX3.0 data that aren't available rather than defaulting to the legacy profile (same point applies to the other endpoints that aren't available in WQX3.0).

At the very least, these warning messages could be more specific to the endpoint - for example, "The Organization data profile is not currently available in WQX3.0."

jzemmels · 2024-09-10T17:45:38Z

dataretrieval/wqp.py

+        Describes the type of columns to return with the result dataset.
+        Most recent WQX3 profiles include 'fullPhysChem', 'narrow', and
+        'basicPhysChem'. Legacy profiles include 'resultPhysChem',
+        'biological', and 'narrowResult'. 
    siteid: string


Could these parameters be added to the documentation of the other WQP functions? Seems like it would be annoying to look at the get_results documentation to know how to use the what_* functions. Plus, the claim in the other functions that they "accept the same parameters" as get_results is not true; for example, none of them need the dataProfile argument. Copying + pasting the relevant parameters to the other functions' doc strings is easy.

Good eye. I'm not sure the convention with kwargs documentation, but I can look around (see bottom of this Sphinx doc page: https://pythonhosted.org/an_example_pypi_project/sphinx.html#full-code-exampl). @thodson-usgs, what do you think? I appreciate linking to the webservices guide, which means we don't need to be checking our documentation against the WQP's, but for the uninitiated, wading through the webservices guide (which covers building urls but isn't exactly specific to using a python function) might be a little confusing. Again, I think I'm going to add this as an issue and let this PR only mess with the wqx3 machinery stuff.

jzemmels · 2024-09-10T17:53:34Z

dataretrieval/wqp.py

+                   'ResultDetectionQuantitationLimit',
+                   'BiologicalMetric']
+
+def get_results(ssl_check=True, legacy=True, **kwargs):
    """Query the WQP for results.


I think the documentation here and in the other what_* functions could be clearer. For example, what are the "results" that one would expect to obtain? Or what does it mean to search for sites/projects/organizations/activities/etc. "within a region with specific data?" For what purpose would someone use these functions? A simple description of these WQP endpoints would be helpful.

I agree, the definitions of the functions are a bit murky. We can use dataRetrieval as a good example: https://doi-usgs.github.io/dataRetrieval/reference/wqpSpecials.html, though also quite light on specifics.

thodson-usgs

When attempting to use the new profile, I got a looooong delay then an error message. Let's table the other discussions until the basic functionality is working.

jzemmels · 2024-09-16T17:25:53Z

When attempting to use the new profile, I got a looooong delay then an error message. Let's table the other discussions until the basic functionality is working.

Interesting, the WQX 3.0 profiles seemed to be working for me earlier but aren't now. I just tried to perform the same query on the WQP beta webpage and also ran into issues. It might be a timeout problem?

thodson-usgs · 2024-09-16T18:51:06Z

Yeah, seems to be an issue on the service side. Let's add another warning for "legacy=False"; otherwise, I'm fine with this as a first crack PR.

lstanish-usgs · 2024-09-16T19:29:02Z

@jzemmels can you share the tests you ran that gave you errors? I want to share them with the services development teams.

jzemmels · 2024-09-16T19:54:53Z

@jzemmels can you share the tests you ran that gave you errors? I want to share them with the services development teams.

Sure, this call threw an error.
wqp.get_results(countrycode="US", statecode="US:21", mimeType="csv", providers=["STORET","NWIS"], dataProfile="fullPhysChem", startDateLo="01-01-2024", startDateHi="09-01-2024", legacy=False)

And this call returned an empty data frame after about a minute.
wqp.get_results(countrycode="US", statecode="US:21", mimeType="csv", providers=["STORET","NWIS"], dataProfile="narrow", startDateLo="01-01-2024", startDateHi="09-01-2024", legacy=False)

The equivalent URL couldn't complete the download, so I assume its somehow related to the empty data frame.
https://www.waterqualitydata.us/wqx3/Result/search?countrycode=US&statecode=US%3A21&mimeType=csv&providers=STORET%3BNWIS&dataProfile=narrow&startDateLo=01-01-2024&startDateHi=09-01-2024

thodson-usgs

I approve, though I added some suggestions in a PR to your fork, which you might want to merge first.
Nice work.

Docstrings and linting

Fix mimetype test

first crack at changing the url

b499253

ehinman requested a review from thodson-usgs August 16, 2024 23:00

thodson-usgs reviewed Aug 17, 2024

View reviewed changes

ehinman added 2 commits August 20, 2024 18:03

trying to avoid a legacy=True argument in favor of unique dataprofiles

41b3270

check services

c5839c1

add legacy back in, adjust all functions and tests

9238e50

ehinman marked this pull request as ready for review August 22, 2024 23:16

ehinman self-assigned this Aug 27, 2024

ehinman requested a review from jzemmels August 27, 2024 17:22

ehinman added 3 commits September 4, 2024 14:39

add test, remove old wqx warning, add links to beta page

7c7ba2d

Merge branch 'master' into wqx3-work

2603d5d

fix type error syntax

13bb200

ehinman requested a review from thodson-usgs September 4, 2024 20:00

jzemmels reviewed Sep 10, 2024

View reviewed changes

Docstrings and linting

9061f4c

thodson-usgs reviewed Sep 16, 2024

View reviewed changes

Add wqx3 warning

85f2791

thodson-usgs approved these changes Sep 16, 2024

View reviewed changes

thodson-usgs and others added 11 commits September 16, 2024 17:11

Quick fix

9bbae87

Reorder services

83444be

Merge pull request #1 from thodson-usgs/wqx3-work

6723b5e

Docstrings and linting

try to fix linting issue

c2b4d12

Fix mimetype test

089354c

another fix

3a8c818

Merge pull request #2 from thodson-usgs/wqx3-work

91947ad

Fix mimetype test

Fix test

ec36997

Return csv to kwargs

ef09e3d

Return wqx3 test

40a2b46

Merge branch 'DOI-USGS:master' into wqx3-work

57f4ab7

thodson-usgs merged commit 9f09525 into DOI-USGS:master Sep 17, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First crack at changing the url for one function #152

First crack at changing the url for one function #152

ehinman commented Aug 16, 2024 •

edited

Loading

thodson-usgs left a comment

thodson-usgs Aug 17, 2024

ehinman commented Aug 20, 2024

thodson-usgs commented Aug 21, 2024 •

edited

Loading

ehinman commented Aug 21, 2024

ehinman commented Aug 21, 2024

ehinman commented Aug 30, 2024

thodson-usgs commented Aug 30, 2024

jzemmels commented Aug 30, 2024 •

edited

Loading

jzemmels commented Aug 30, 2024 •

edited

Loading

jzemmels commented Aug 30, 2024

thodson-usgs commented Sep 3, 2024 •

edited

Loading

ehinman commented Sep 4, 2024

jzemmels left a comment

jzemmels Sep 9, 2024

jzemmels Sep 10, 2024

ehinman Sep 17, 2024 •

edited

Loading

jzemmels Sep 10, 2024

thodson-usgs Sep 16, 2024

lstanish-usgs Sep 16, 2024

jzemmels Sep 10, 2024

jzemmels Sep 10, 2024

ehinman Sep 17, 2024

jzemmels Sep 10, 2024

ehinman Sep 17, 2024

thodson-usgs left a comment

jzemmels commented Sep 16, 2024

thodson-usgs commented Sep 16, 2024

lstanish-usgs commented Sep 16, 2024

jzemmels commented Sep 16, 2024

thodson-usgs left a comment

First crack at changing the url for one function #152

First crack at changing the url for one function #152

Conversation

ehinman commented Aug 16, 2024 • edited Loading

thodson-usgs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehinman commented Aug 20, 2024

thodson-usgs commented Aug 21, 2024 • edited Loading

ehinman commented Aug 21, 2024

ehinman commented Aug 21, 2024

ehinman commented Aug 30, 2024

thodson-usgs commented Aug 30, 2024

jzemmels commented Aug 30, 2024 • edited Loading

jzemmels commented Aug 30, 2024 • edited Loading

jzemmels commented Aug 30, 2024

thodson-usgs commented Sep 3, 2024 • edited Loading

ehinman commented Sep 4, 2024

jzemmels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehinman Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thodson-usgs left a comment

Choose a reason for hiding this comment

jzemmels commented Sep 16, 2024

thodson-usgs commented Sep 16, 2024

lstanish-usgs commented Sep 16, 2024

jzemmels commented Sep 16, 2024

thodson-usgs left a comment

Choose a reason for hiding this comment

ehinman commented Aug 16, 2024 •

edited

Loading

thodson-usgs commented Aug 21, 2024 •

edited

Loading

jzemmels commented Aug 30, 2024 •

edited

Loading

jzemmels commented Aug 30, 2024 •

edited

Loading

thodson-usgs commented Sep 3, 2024 •

edited

Loading

ehinman Sep 17, 2024 •

edited

Loading