Preserve Samples CSV identifiers through shared parsing - #419
Merged
Conversation
Collaborator
|
Thanks @tevinch. I agree, this is the correct behavior but will proceed caustiously since this is a breaking change. |
thodson-usgs
added a commit
to thodson-usgs/dataretrieval-python
that referenced
this pull request
Sep 9, 2026
Extends DOI-USGS#419 from the two Samples getters to every delimited response the package parses, and applies one rule to all of them. A code column is named by a ``code`` suffix, the RDB abbreviation ``_cd``, or a code vocabulary in the name (``identifier``, ``huc``, ``fips``); a name ending in ``count`` is a tally and keeps numeric inference. ``dataretrieval._csv`` owns that rule, exposing ``read_code_csv`` for a CSV body and ``code_columns`` for a caller that parses the body itself. Fixed as a result: - ``waterdata.get_samples`` returned ``USGSpcode`` 00060 as 60 and HUC12 070700050502 as 70700050502 (the DOI-USGS#419 fix, unchanged in effect). - ``nwis.get_info`` / ``what_sites`` / ``get_record(service="site")`` returned ``huc_cd`` 02060005 as 2060005, and dropped the zeros from ``state_cd``, ``county_cd`` and ``district_cd``. ``rdb.read_rdb`` now detects code columns from the RDB header rather than the caller enumerating them, so ``_NWIS_RDB_DTYPES`` holds only the names the rule cannot infer. - WQP's ``AlternateLocation_IdentifierCount`` was read as text because "Identifier" appears in its name, giving one WQX3 column two dtypes depending on which adapter fetched it. ``nwdc`` drops its hard-coded ``{huc12_id: str}`` for the shared reader, which covers that column by name. The OGC getters (``waterdata``, ``ngwmn``), NWIS JSON, NLDI and StreamStats were audited and need no change: their codes arrive as JSON strings and numeric coercion there is a fixed list of measurement columns. tests/data/samples_results.txt and tests/data/wqp3_results.txt were recorded through pandas and had already lost their zeros (pcode 61, HUC8 7090002), so neither could fail on this bug. Both are re-recorded from their live queries and now assert the padded values. Also: the shared reader's tests move to tests/_csv_test.py, WQP's parser test becomes a getter-level regression, and NEWS records the behavior change per service plus the same correction shipped undocumented for WQP in 1.2.0.
thodson-usgs
added a commit
that referenced
this pull request
Sep 9, 2026
Extends #419 from the two Samples getters to every delimited response the package parses, and applies one rule to all of them. A code column is named by a ``code`` suffix, the RDB abbreviation ``_cd``, or a code vocabulary in the name (``identifier``, ``huc``, ``fips``); a name ending in ``count`` is a tally and keeps numeric inference. ``dataretrieval._csv`` owns that rule, exposing ``read_code_csv`` for a CSV body and ``code_columns`` for a caller that parses the body itself. Fixed as a result: - ``waterdata.get_samples`` returned ``USGSpcode`` 00060 as 60 and HUC12 070700050502 as 70700050502 (the #419 fix, unchanged in effect). - ``nwis.get_info`` / ``what_sites`` / ``get_record(service="site")`` returned ``huc_cd`` 02060005 as 2060005, and dropped the zeros from ``state_cd``, ``county_cd`` and ``district_cd``. ``rdb.read_rdb`` now detects code columns from the RDB header rather than the caller enumerating them, so ``_NWIS_RDB_DTYPES`` holds only the names the rule cannot infer. - WQP's ``AlternateLocation_IdentifierCount`` was read as text because "Identifier" appears in its name, giving one WQX3 column two dtypes depending on which adapter fetched it. ``nwdc`` drops its hard-coded ``{huc12_id: str}`` for the shared reader, which covers that column by name. The OGC getters (``waterdata``, ``ngwmn``), NWIS JSON, NLDI and StreamStats were audited and need no change: their codes arrive as JSON strings and numeric coercion there is a fixed list of measurement columns. tests/data/samples_results.txt and tests/data/wqp3_results.txt were recorded through pandas and had already lost their zeros (pcode 61, HUC8 7090002), so neither could fail on this bug. Both are re-recorded from their live queries and now assert the padded values. Also: the shared reader's tests move to tests/_csv_test.py, WQP's parser test becomes a getter-level regression, and NEWS records the behavior change per service plus the same correction shipped undocumented for WQP in 1.2.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #411.
Both Samples CSV getters currently infer zero-padded codes and identifiers as numbers, so
00060becomes60. Move WQP's two-pass CSV parser into the service-neutraldataretrieval._csvmodule and reuse it from both Samples getters. WQP retains its existing parsing behavior and private helper import.Samples explicitly leaves
AlternateLocation_IdentifierCountunder numeric inference: despite its name, it is a count, not an identifier. Public getter regressions cover parameter/HUC/FIPS codes, numeric-looking location and organization identifiers, measurement/count dtypes, missing measurements, original date columns, UTC shaping/sorting, URLs, and metadata. Getter signatures and return tuples are unchanged. The dtype correction is documented in NEWS and the Samples docstrings.Validation on Python 3.12 / pandas 2.2.3:
coverage run -m pytest tests/: 1,142 passed, 12 live tests deselected; coverage 98.93% with branch measurement enabled (98.9% gate).mypy,ruff check .,ruff format --check .: passed.lint-imports: 8 contracts kept; xenon and complexipy passed.The two new public-getter cases failed before the identifier fix; the count-field assertions also failed before its inference exception. WQP's regression remains passing.