-
Notifications
You must be signed in to change notification settings - Fork 41
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
Adjust gwlevels URL services #147
Conversation
I don't know. Did you check the service's doc and check whether you could provide a bbox to through the REST API? |
Ok, I'll take a look. I probably did something wrong. Hey, do you have insights for why the tests are failing for gwlevels? The basic error message for all is:
I'm not sure how I messed up the mocking structure. Thanks! |
I don't see anything obvious. Did you verify that the url in the mock matches that in the metadata from the same query? |
Fixed it! Whew. I was at a loss for a day. Simple fix. |
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.
One suggestion
dataretrieval/nwis.py
Outdated
@@ -429,17 +430,28 @@ def get_gwlevels( | |||
.. doctest:: | |||
|
|||
>>> # Get groundwater levels for site 434400121275801 | |||
>>> df, md = dataretrieval.nwis.get_gwlevels(sites='434400121275801') | |||
>>> df, md = dataretrieval.nwis.get_gwlevels(site_no='434400121275801') |
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.
Why did you decide to break the API? sites
-> site_no
?
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.
Okay, I see later you catch sites
and pass them to site_no
. In that case, I might keep sites
as the recommended parameter.
keep API consistent Co-authored-by: Timothy Hodson <[email protected]>
Do we need lines 444-445 in nwis.py, then? I guess it's served in line 451. This is my first time messing with kwargs. |
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.
@ehinman regarding bbox queries, they are kinda tricky. You enter the coordinates starting in the west and end in the north, so you move in a counterclockwise direction: Ex. -176, 34, -174, 36. Description here: https://waterservices.usgs.gov/docs/site-service/site-service-details/ |
This PR moves the 'gwlevels' service from the 'waterservices' umbrella to the 'waterdata' umbrella, since the 'waterservices' endpoint is going away as soon as next week! It ensures that users who use the 'waterservices' inputs like 'startDT', 'endDT', and 'sites' are still able to return gwlevel queries. I updated the tests for this function as well.
The edit to the notebook was to change the plotting syntax to avoid an error with using the index. Looks like other cells were slightly affected by spacing and such.
Question: should the user be able to use a bounding box with this query? I tried to get it to work and could not figure out the correct syntax. I'm not sure if I broke it with the URL change, or if it has always been a bit tricky to figure out.