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

feat(geopandas support): return GeoDataFrame if geopandas is installed #143

Merged
merged 3 commits into from
Jul 19, 2024

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.

@@ -71,6 +77,14 @@ def format_response(
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
11 checks passed
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