Skip to content

Conversation

@jlarsen-usgs
Copy link
Contributor

No description provided.

Copy link
Collaborator

@thodson-usgs thodson-usgs left a comment

Choose a reason for hiding this comment

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

Very nice. Even better if you added a unit test, but I'll approve it either way.

Otherwise, I had one comment.

if service == 'peaks':
df = preformat_peaks_response(df)

if gpd is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this remove the original lat/lon column?
If so (and perhaps either way), we should make this conversion optional, so that we don't break existing codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This maintains the original lat/long columns. This PR just builds a geometry field from that data and creates a geodataframe. I can add a unit test that checks the return type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated tests for geopandas support

Copy link
Collaborator

@thodson-usgs thodson-usgs left a comment

Choose a reason for hiding this comment

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

looks good!

@thodson-usgs thodson-usgs merged commit 64a575d into DOI-USGS:master Jul 19, 2024
aaraney added a commit to aaraney/dataretrieval-python that referenced this pull request Oct 25, 2024
The nwis site service (and the USGS more broadly) uses the NAD83 crs.

64a575d introduced a bug that hard coded the nwis module's  crs to
EPSG:4236 -- Hu Tzu Shan 1950 (https://epsg.io/4236). I guarantee
64a575d meant to hard code _EPSG:4326_ -- WGS84. It appears it was fat
fingered and missed in DOI-USGS#143's PR review (it happens to the best of us:)).

In either case, EPSG:4326 is still _technically_ the wrong crs. For example:

https://waterservices.usgs.gov/nwis/site?sites=01013500&siteOutput=Expanded&format=rdb

returns
```
USGS 01013500 ... NAD83
```
for the field `coord_datum_cd  -- Latitude-longitude datum`.
So EPSG:4269 -- NAD83 is the correct crs.

There is room for discussion if NAD83 vs WGS 84 really matters in the
conterminous US however for Alaska errors in both the x and y direction
can be upwards of 1 meter in either direction.

BREAKING CHANGE: v1.0.10 is the only affected version. The GeoDataFrames
returned from the following functions now correctly have a crs of
EPSG:4269.
dataretrieval.nwis.get_qwdata
dataretrieval.nwis.get_discharge_measurements
dataretrieval.nwis.get_discharge_peaks
dataretrieval.nwis.get_gwlevels
dataretrieval.nwis.get_stats
dataretrieval.nwis.get_info
dataretrieval.nwis.get_pmcodes
dataretrieval.nwis.get_water_use
dataretrieval.nwis.get_ratings
dataretrieval.nwis.what_sites
dataretrieval.nwis.get_dv
dataretrieval.nwis.get_iv
thodson-usgs pushed a commit that referenced this pull request Oct 25, 2024
The nwis site service (and the USGS more broadly) uses the NAD83 crs.

64a575d introduced a bug that hard coded the nwis module's  crs to
EPSG:4236 -- Hu Tzu Shan 1950 (https://epsg.io/4236). I guarantee
64a575d meant to hard code _EPSG:4326_ -- WGS84. It appears it was fat
fingered and missed in #143's PR review (it happens to the best of us:)).

In either case, EPSG:4326 is still _technically_ the wrong crs. For example:

https://waterservices.usgs.gov/nwis/site?sites=01013500&siteOutput=Expanded&format=rdb

returns
```
USGS 01013500 ... NAD83
```
for the field `coord_datum_cd  -- Latitude-longitude datum`.
So EPSG:4269 -- NAD83 is the correct crs.

There is room for discussion if NAD83 vs WGS 84 really matters in the
conterminous US however for Alaska errors in both the x and y direction
can be upwards of 1 meter in either direction.

BREAKING CHANGE: v1.0.10 is the only affected version. The GeoDataFrames
returned from the following functions now correctly have a crs of
EPSG:4269.
dataretrieval.nwis.get_qwdata
dataretrieval.nwis.get_discharge_measurements
dataretrieval.nwis.get_discharge_peaks
dataretrieval.nwis.get_gwlevels
dataretrieval.nwis.get_stats
dataretrieval.nwis.get_info
dataretrieval.nwis.get_pmcodes
dataretrieval.nwis.get_water_use
dataretrieval.nwis.get_ratings
dataretrieval.nwis.what_sites
dataretrieval.nwis.get_dv
dataretrieval.nwis.get_iv
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