Skip to content

Conversation

@elbeejay
Copy link
Contributor

@elbeejay elbeejay commented Dec 13, 2022

PR to close issue #60

Issue is well described in #60. A test has been added that will be expanded in the future when functionality is fixed.

Fix is to defer setting the time objects when calling pd.read_json() by setting the parameter convert_dates=False in that function call. Then once the final data frame is constructed, the datetime information is converted from a string to a datetime.datetime object. So the end result should be the same as it was before, just the internal conversion happens in a different order. I wrote a pair of tests based on the error described in the issue to confirm the original behavior / typing using sites for which the old code worked as well as a test where it failed. Then with the proposed changes both tests pass.

PR should also close issue #61

Points raised around time handling there prompted the creation of a "User Guide" section in the documentation where little bits of code and explanation can be added to discuss things like the timestamps and how dataretrieval attempts to handle datetimes.

Change in UTC localization

As a result of some of the poking around related to #61, I found that while dataretrieval attempts to UTC-localize the timestamps (in nwis.format_response), this often wasn't happening for the multi-site case. This resulted in some of the timestamp stuff discussed in #61, such as the pytz.FixedOffset portions of the timestamps. Since this PR more manually converts the timestamps now (rather than letting pd.read_json() try to do it), we gained the opportunity to set the times in UTC at that point. So now even the multi-site returns should have timestamps that are formatted in UTC by default. This seems like it was the intent of the package all along, but just wasn't always happening.

@elbeejay elbeejay linked an issue Dec 16, 2022 that may be closed by this pull request
@elbeejay elbeejay marked this pull request as ready for review December 16, 2022 18:05
@elbeejay
Copy link
Contributor Author

This PR should be good to go, the failing test involves a service that wasn't modified as part of this PR, so it may just be down temporarily. Aim of this PR is to address issues #60 and #61, so @SorooshMani-NOAA and @ddileonardo please feel free to check out the PR description and code and I welcome any feedback you may have.

@SorooshMani-NOAA
Copy link

@elbeejay it has fixed the original issue. Maybe unrelated, but when I try to get the data from all the sites in Maryland, it gives me an error related to parsing the JSON. My assumption is maybe the site is too large and the webAPI responds with an error (?) which fails to be parsed. Is there any way to have better error messages? e.g. area is too large for fetching data?

@thodson-usgs thodson-usgs mentioned this pull request Dec 19, 2022
@elbeejay
Copy link
Contributor Author

@elbeejay it has fixed the original issue. Maybe unrelated, but when I try to get the data from all the sites in Maryland, it gives me an error related to parsing the JSON. My assumption is maybe the site is too large and the webAPI responds with an error (?) which fails to be parsed. Is there any way to have better error messages? e.g. area is too large for fetching data?

Glad to hear the original issue is handled by this fix. The new issue you've run into when trying to fetch data for all sites in Maryland is that the query size is too large; in #65 a more informative error message is proposed (as the current one is, as you say, not very informative).

submmitted -> submitted typo fix
@thodson-usgs thodson-usgs merged commit 3b13700 into DOI-USGS:master Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

3 participants