From 1dd136f4de4e49d349ba409c8c9d635504cbf036 Mon Sep 17 00:00:00 2001 From: Arpit Jain Date: Fri, 17 Jul 2026 06:03:15 +0900 Subject: [PATCH 1/6] fix(nwis): handle empty peaks response instead of raising KeyError nwis.get_discharge_peaks / get_record(service="peaks") against a site with no annual-peak data returns a peaks RDB body of comment lines only, which read_rdb parses to a column-less empty DataFrame. format_response runs preformat_peaks_response before its own empty-frame check, and that function's first statement pops "peak_dt", so the empty case raised KeyError('peak_dt') instead of returning an empty frame. This is the same empty-result contract fixed for the other services in issue #171; peaks was missed because it is preformatted first. Return the frame unchanged when peak_dt is absent so the empty-frame path in format_response handles it and callers can check df.empty. Adds a regression test alongside the existing #171 coverage. Signed-off-by: Arpit Jain --- dataretrieval/nwis.py | 7 +++++++ tests/nwis_test.py | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 25e9cf8f4..255ebbe85 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -196,6 +196,13 @@ def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: The formatted data frame """ + if "peak_dt" not in df.columns: + # An empty peaks response (e.g. "No sites found") parses to a + # column-less frame, so there is no peak_dt to reformat. Return it + # unchanged and let format_response's empty-frame path handle it, + # matching how the other services treat empty results (issue #171). + return df + df["datetime"] = pd.to_datetime(df.pop("peak_dt"), errors="coerce") df.dropna(subset=["datetime"], inplace=True) return df diff --git a/tests/nwis_test.py b/tests/nwis_test.py index 905ed8db0..f95b40765 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -11,6 +11,7 @@ from dataretrieval.nwis import ( NWIS_Metadata, _read_rdb, + format_response, get_discharge_measurements, get_gwlevels, get_iv, @@ -303,3 +304,20 @@ def test_no_sites_flows_through_format_response(self): df = _read_rdb(no_sites_rdb) assert isinstance(df, pd.DataFrame) assert df.empty + + def test_no_peaks_flows_through_format_response(self): + """The 'peaks' service takes an extra formatting step + (preformat_peaks_response) before the empty-frame check, so an empty + peaks response has to survive that too. Previously this raised + KeyError('peak_dt'); now it returns an empty frame like the other + services (same empty-result contract as issue #171). + """ + no_peaks_rdb = ( + "# //Output-Format: RDB\n" + "# //Response-Status: OK\n" + "# //Response-Message: No sites found matching all criteria\n" + ) + df = _read_rdb(no_peaks_rdb) + df = format_response(df, service="peaks") + assert isinstance(df, pd.DataFrame) + assert df.empty From 751d3b326d99fb60a76d96d7774e25f58a05eb6c Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Thu, 16 Jul 2026 16:22:32 -0500 Subject: [PATCH 2/6] test(nwis): parse with raw read_rdb in empty-peaks regression test The empty-peaks test parsed with _read_rdb, which already runs format_response(service=None); the real get_discharge_peaks path uses the raw read_rdb parser followed by format_response(service="peaks"). Switch to read_rdb so the test exercises the actual call path without the redundant format pass. Signed-off-by: thodson-usgs Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/nwis_test.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/nwis_test.py b/tests/nwis_test.py index f95b40765..9bb24bf75 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -20,6 +20,7 @@ get_record, get_water_use, preformat_peaks_response, + read_rdb, ) START_DATE = "2018-01-24" @@ -317,7 +318,9 @@ def test_no_peaks_flows_through_format_response(self): "# //Response-Status: OK\n" "# //Response-Message: No sites found matching all criteria\n" ) - df = _read_rdb(no_peaks_rdb) + # Mirror get_discharge_peaks: raw read_rdb, then the peaks-specific + # format_response (not _read_rdb, which formats with service=None). + df = read_rdb(no_peaks_rdb) df = format_response(df, service="peaks") assert isinstance(df, pd.DataFrame) assert df.empty From c49702facf327121626dee508e310368dbb82237 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Wed, 26 Aug 2026 16:36:11 -0500 Subject: [PATCH 3/6] fix(nwis): keep peaks whose date is only partly known NWIS zero-fills the unknown part of a historical peak's date -- YYYY-MM-00 when the day is not known, YYYY-00-00 when the month is not either, the Bd and Bm peak_cd qualifiers. Neither parses as a date, so to_datetime coerced both to NaT and the dropna below deleted the row along with its discharge value. The loss is silent and biased toward the records that matter most, because the imprecisely dated peaks are the old ones: site 14105700 lost 20 of its 167 peaks, among them an 1859 flood of 847,000 ft3/s, and 06934500 lost its 1844 peak of 700,000 ft3/s. Measured across six sites, 24 of 672 peaks were being dropped; none are now. Pin each to the start of the period that is known. waterdata.get_peaks(), the replacement this facade points at, resolves the same records the same way -- it dates a year-only peak to 1 January and flags it [MONTHUNKNOWN] -- so the two agree during the migration window. Filtered to discharge, legacy and modern now return the same 167 rows for 14105700. A peak with no peak_dt at all is still dropped: there is no period to pin it to, and no way to place it on the datetime index format_response builds. That is the case the existing test pins, and it stays green. Permitted by ADR 0005, which allows correctness fixes on the deprecated nwis facade. Co-Authored-By: Claude Opus 5 (1M context) --- NEWS.md | 2 ++ dataretrieval/nwis.py | 28 +++++++++++++++++++++++++++- tests/nwis_test.py | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 6dfb3ba9e..363f510e4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,5 @@ +**08/26/2026:** **Bug fix:** `nwis.get_discharge_peaks` and `nwis.get_record(service='peaks')` silently discarded every peak whose date is only partly known. NWIS zero-fills the unknown part -- `YYYY-MM-00` when the day is not known, `YYYY-00-00` when the month is not either (the `Bd` and `Bm` `peak_cd` qualifiers) -- and neither parses as a date, so `preformat_peaks_response` coerced both to `NaT` and then dropped the row along with its discharge value. These are real peaks, and disproportionately a site's largest: site 14105700 lost 20 of its 167 peaks, among them an 1859 flood of 847,000 ft3/s, and 06934500 lost its 1844 peak of 700,000 ft3/s. Each is now dated to the start of the period that *is* known, which is how `waterdata.get_peaks()` resolves the same records -- it dates a year-only peak to 1 January and flags it `[MONTHUNKNOWN]`. A peak carrying no `peak_dt` at all is still dropped, since there is no period to place it on the datetime index `format_response` builds. **Behavior change:** peaks queries return more rows than before, and the recovered rows carry an approximate date -- read `peak_cd` for the `Bd`/`Bm` qualifier to tell them apart. + **08/25/2026:** Removed `dataretrieval.ogc.retry`, which only re-exported private helpers. Deprecated `dataretrieval.ogc.interruptions`; import exceptions from `dataretrieval` or `dataretrieval.interruptions` instead. The old path will be removed in a future major release, no earlier than 2027-08-25. **08/20/2026:** The `state` filter now accepts the five US territories. `dataretrieval.codes.states` held the 50 states and DC, so `ngwmn.get_sites(state='Puerto Rico')`, `waterdata.get_monitoring_locations(state_name='Puerto Rico')` via the unified `state`, and `nwdc.get_wateruse(state='PR')` were refused locally -- while all three services carry the data (NGWMN answers with 36 Puerto Rico monitoring locations, the Water Data monitoring-locations collection returns Puerto Rico sites, and legacy NWIS lists 1,148 stream sites for `stateCd=PR`). American Samoa, Guam, the Northern Mariana Islands, Puerto Rico and the US Virgin Islands are now in both code tables under their real ANSI/FIPS codes, so every encoding resolves: `'Puerto Rico'`, `'PR'`, `'72'` and `'US:72'` all normalize alike. **Behavior change:** a territory that used to raise `ValueError` now produces a request. A value the table genuinely does not hold still fails fast. diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 44d74828f..1f6b68e58 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -204,6 +204,25 @@ def format_response( return df.sort_index() +def _peak_datetimes(peak_dt: pd.Series) -> pd.Series: + """Parse a ``peak_dt`` column, keeping peaks whose date is partly unknown. + + NWIS writes ``YYYY-MM-00`` when the day of a historical peak is not known + and ``YYYY-00-00`` when the month is not either -- the ``Bd`` and ``Bm`` + ``peak_cd`` qualifiers. Neither parses as a date, so each is pinned to the + start of the period that *is* known. ``waterdata.get_peaks()`` resolves the + same records the same way, dating a year-only peak to 1 January and + flagging it ``[MONTHUNKNOWN]``. + + A ``peak_dt`` that is blank or absent stays ``NaT``: there is no period to + pin it to, and :func:`preformat_peaks_response` drops it. + """ + text = peak_dt.astype("string").str.strip() + text = text.str.replace(r"^(\d{4})-00-", r"\1-01-", regex=True) + text = text.str.replace(r"^(\d{4}-\d{2})-00$", r"\1-01", regex=True) + return pd.to_datetime(text, errors="coerce") + + def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: """Format the datetime column of the 'peaks' service response. @@ -217,8 +236,15 @@ def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: df: ``pandas.DataFrame`` The formatted data frame. + Notes + ----- + Peaks whose day or month is unknown are dated to the start of the known + period rather than discarded; see :func:`_peak_datetimes`. Rows with no + ``peak_dt`` at all are dropped, since they cannot be placed on the + datetime index :func:`format_response` builds. + """ - df["datetime"] = pd.to_datetime(df.pop("peak_dt"), errors="coerce") + df["datetime"] = _peak_datetimes(df.pop("peak_dt")) df.dropna(subset=["datetime"], inplace=True) return df diff --git a/tests/nwis_test.py b/tests/nwis_test.py index 9c424fa01..03efdfda8 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -92,6 +92,40 @@ def test_preformat_peaks_response(): assert df["datetime"].isna().sum() == 0 +@pytest.mark.parametrize( + ("peak_dt", "expected"), + [ + ("1878-06-12", "1878-06-12"), # fully known + ("1844-06-00", "1844-06-01"), # day unknown (peak_cd Bd) + ("1858-00-00", "1858-01-01"), # month unknown (peak_cd Bm) + ], +) +def test_preformat_peaks_response_keeps_partial_dates(peak_dt, expected): + """NWIS zero-fills the unknown part of a historical peak's date, and those + are real peaks -- often a site's largest. They must be pinned to the start + of the known period, not coerced to NaT and dropped. ``waterdata.get_peaks`` + resolves the same records the same way. + """ + df = pd.DataFrame({"peak_dt": [peak_dt], "peak_va": [563000]}) + + df = preformat_peaks_response(df) + + assert len(df) == 1, f"{peak_dt} was dropped" + assert df["datetime"].iloc[0] == pd.Timestamp(expected) + assert df["peak_va"].iloc[0] == 563000 + + +def test_preformat_peaks_response_drops_dateless_peaks(): + """A peak with no date at all has no period to pin it to, so it cannot go + on the datetime index format_response builds and is still dropped. + """ + df = pd.DataFrame({"peak_dt": ["2000-03-22", None, ""], "peak_va": [1, 2, 3]}) + + df = preformat_peaks_response(df) + + assert df["peak_va"].tolist() == [1] + + class TestDeprecationWarnings: """Verify per-function DeprecationWarning fires with the right replacement. From d9d090c4177c056d619b4fbf93cbc75a44e2f52e Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Wed, 26 Aug 2026 16:40:00 -0500 Subject: [PATCH 4/6] fix(nwis): narrow the empty-peaks guard to actually-empty frames Review follow-ups on the empty-peaks guard. The guard fired on any frame missing peak_dt, including a non-empty one from a truncated or altered RDB header. Such a frame is malformed rather than empty, and was being returned silently without its datetime index where it used to raise. Require df.empty too, and pin it with a test. Correct the regression test's docstring, which said the empty responses get_discharge_peaks sees are caught earlier as NoSitesError. That check fires only on a body starting "No sites/data" -- what the live service happens to send today -- so a comment-only RDB does reach the guarded line. As written the docstring read as "this guard is unreachable", inviting its deletion. Widen the test class docstring to cover both arms it now holds, document the pass-through in preformat_peaks_response's public docstring, and add the NEWS entry this behavior change warrants. Co-Authored-By: Claude Opus 5 (1M context) --- NEWS.md | 2 ++ dataretrieval/nwis.py | 12 ++++++++++-- tests/nwis_test.py | 29 ++++++++++++++++++++++------- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/NEWS.md b/NEWS.md index 6dfb3ba9e..f025a9992 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,5 @@ +**08/26/2026:** **Bug fix:** `nwis.format_response(df, service='peaks')` and `nwis.preformat_peaks_response` raised `KeyError('peak_dt')` on an empty peaks response instead of returning an empty frame. Both are public, and every other service already treated an empty result as a legitimate empty frame rather than an error (issue #171); the peaks arm slipped through because it reformats the datetime column before the empty-frame check. Callers can now check `df.empty` rather than catching an exception. A *non-empty* frame with no `peak_dt` column is malformed rather than empty, and still raises. + **08/25/2026:** Removed `dataretrieval.ogc.retry`, which only re-exported private helpers. Deprecated `dataretrieval.ogc.interruptions`; import exceptions from `dataretrieval` or `dataretrieval.interruptions` instead. The old path will be removed in a future major release, no earlier than 2027-08-25. **08/20/2026:** The `state` filter now accepts the five US territories. `dataretrieval.codes.states` held the 50 states and DC, so `ngwmn.get_sites(state='Puerto Rico')`, `waterdata.get_monitoring_locations(state_name='Puerto Rico')` via the unified `state`, and `nwdc.get_wateruse(state='PR')` were refused locally -- while all three services carry the data (NGWMN answers with 36 Puerto Rico monitoring locations, the Water Data monitoring-locations collection returns Puerto Rico sites, and legacy NWIS lists 1,148 stream sites for `stateCd=PR`). American Samoa, Guam, the Northern Mariana Islands, Puerto Rico and the US Virgin Islands are now in both code tables under their real ANSI/FIPS codes, so every encoding resolves: `'Puerto Rico'`, `'PR'`, `'72'` and `'US:72'` all normalize alike. **Behavior change:** a territory that used to raise `ValueError` now produces a request. A value the table genuinely does not hold still fails fast. diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 05715d62c..86b32824c 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -217,10 +217,18 @@ def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: df: ``pandas.DataFrame`` The formatted data frame. + Notes + ----- + An empty frame with no ``peak_dt`` column is returned unchanged, so that + an empty peaks response reaches :func:`format_response`'s empty-frame path + rather than raising ``KeyError``. + """ - if "peak_dt" not in df.columns: + if df.empty and "peak_dt" not in df.columns: # An empty response parses to a column-less frame; return it so - # format_response's empty-frame path handles it like every other service. + # format_response's empty-frame path handles it like every other + # service. A non-empty frame missing peak_dt is malformed, not empty, + # and still raises. return df df["datetime"] = pd.to_datetime(df.pop("peak_dt"), errors="coerce") diff --git a/tests/nwis_test.py b/tests/nwis_test.py index e11d816dd..f479e9517 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -287,11 +287,12 @@ def test_set_metadata_info_countyCd(self, httpx_mock): class TestReadRdb: - """Tests for the NWIS-specific _read_rdb wrapper. + """Tests for the NWIS-specific parse-then-format path. The format-agnostic parser is exercised in tests/rdb_test.py; this - class pins the wrapper-specific contract — that an empty parser - result flows through format_response without crashing (issue #171). + class pins the NWIS-specific contract — that an empty parser result + flows through format_response without crashing (issue #171), on the + plain arm via _read_rdb and on the peaks arm via format_response. """ def test_no_sites_flows_through_format_response(self): @@ -315,10 +316,13 @@ def test_no_peaks_flows_through_format_response(self): The peaks arm runs ``preformat_peaks_response`` before the "datetime not in columns" check, and that function popped ``peak_dt`` unconditionally, so a column-less frame raised ``KeyError`` where every - other service returned an empty frame (issue #171's contract). Both - functions are public, so this is reachable directly; the empty - responses ``get_discharge_peaks`` itself sees are caught earlier as - ``NoSitesError``. + other service returned an empty frame (issue #171's contract). + + Both functions are public API, so any caller parsing a peaks RDB + reaches this. It is not dead code guarded by ``NoSitesError``: that + check in ``_querying`` fires only on a body starting "No sites/data", + which is what the live service happens to send today -- a comment-only + RDB reaches the guarded line instead. """ no_peaks_rdb = ( "# //Output-Format: RDB\n" @@ -332,6 +336,17 @@ def test_no_peaks_flows_through_format_response(self): assert isinstance(df, pd.DataFrame) assert df.empty + def test_malformed_peaks_frame_still_raises(self): + """Only an *empty* peaks frame is a legitimate empty result. A + non-empty frame with no ``peak_dt`` column is a malformed response -- + a truncated or altered RDB header -- and must stay loud rather than be + returned silently without its datetime index. + """ + df = pd.DataFrame({"peak_va": [1000]}) + + with pytest.raises(KeyError, match="peak_dt"): + format_response(df, service="peaks") + class TestGetRecordDispatch: """``get_record`` is a router; each service must reach its own getter. From b141003fa95d30de836a5cff107f05ad6dc00cbe Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Wed, 26 Aug 2026 19:33:59 -0500 Subject: [PATCH 5/6] refactor(nwis): tighten the peaks prose and fold duplicated tests Cleanup pass, no behavior change. The guard's contract was stated three times -- Notes paragraph, inline comment, NEWS entry -- so nothing failed when they drifted. Keep the rendered docstring, since preformat_peaks_response is public API, and cut the inline comment back to the constraint it exists to explain. Drop the [MONTHUNKNOWN] detail from _peak_datetimes: naming the Water Data collection's flag format in a private legacy helper rots the first time that collection changes it. test_preformat_peaks_response duplicated the new dateless test on the same happy-path row, and its assertion -- isna().sum() == 0 after a dropna on that column -- was trivially true and would have passed even if every row were dropped. Fold its one distinct input into the stronger test. Move the malformed-frame test out of TestReadRdb, which parses no RDB in it, so the class docstring can describe its contents again, and hoist the comment-only RDB body both its tests use into one constant. Drop the dtype hints from the peaks call: read_rdb returns early for an all-comment body and never applies them, so they only widened the test's coupling to nwis privates. Co-Authored-By: Claude Opus 5 (1M context) --- dataretrieval/nwis.py | 28 ++++++--------- tests/nwis_test.py | 81 +++++++++++++++++-------------------------- 2 files changed, 42 insertions(+), 67 deletions(-) diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 3697a418f..bf0e0b5db 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -210,12 +210,8 @@ def _peak_datetimes(peak_dt: pd.Series) -> pd.Series: NWIS writes ``YYYY-MM-00`` when the day of a historical peak is not known and ``YYYY-00-00`` when the month is not either -- the ``Bd`` and ``Bm`` ``peak_cd`` qualifiers. Neither parses as a date, so each is pinned to the - start of the period that *is* known. ``waterdata.get_peaks()`` resolves the - same records the same way, dating a year-only peak to 1 January and - flagging it ``[MONTHUNKNOWN]``. - - A ``peak_dt`` that is blank or absent stays ``NaT``: there is no period to - pin it to, and :func:`preformat_peaks_response` drops it. + start of the period that *is* known, as ``waterdata.get_peaks()`` resolves + the same records. """ text = peak_dt.astype("string").str.strip() text = text.str.replace(r"^(\d{4})-00-", r"\1-01-", regex=True) @@ -238,21 +234,17 @@ def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: Notes ----- - An empty frame with no ``peak_dt`` column is returned unchanged, so that - an empty peaks response reaches :func:`format_response`'s empty-frame path - rather than raising ``KeyError``. - - Peaks whose day or month is unknown are dated to the start of the known - period rather than discarded; see :func:`_peak_datetimes`. Rows with no - ``peak_dt`` at all are dropped, since they cannot be placed on the - datetime index :func:`format_response` builds. + An empty frame with no ``peak_dt`` column is returned unchanged, so an + empty peaks response reaches :func:`format_response`'s empty-frame path + instead of raising ``KeyError``. Peaks whose day or month is unknown are + dated to the start of the known period rather than discarded; see + :func:`_peak_datetimes`. Rows carrying no ``peak_dt`` at all are dropped, + having no place on the datetime index :func:`format_response` builds. """ if df.empty and "peak_dt" not in df.columns: - # An empty response parses to a column-less frame; return it so - # format_response's empty-frame path handles it like every other - # service. A non-empty frame missing peak_dt is malformed, not empty, - # and still raises. + # A non-empty frame missing peak_dt is malformed, not empty, and must + # still raise. return df df["datetime"] = _peak_datetimes(df.pop("peak_dt")) diff --git a/tests/nwis_test.py b/tests/nwis_test.py index 64c3c3e1a..ef725509a 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -11,7 +11,6 @@ from dataretrieval import nwis from dataretrieval.exceptions import DataCurrencyWarning from dataretrieval.nwis import ( - _NWIS_RDB_DTYPES, NWIS_Metadata, _read_rdb, format_response, @@ -82,21 +81,8 @@ def test_iv_service_answer(httpx_mock): ], f"iv service returned incorrect index: {df.index.names}" -def test_preformat_peaks_response(): - # make a data frame with a "peak_dt" datetime column - # it will have some nan and none values - data = {"peak_dt": ["2000-03-22", np.nan, None], "peak_va": [1000, 2000, 3000]} - # turn data into dataframe - df = pd.DataFrame(data) - # run preformat function - df = preformat_peaks_response(df) - # assertions - assert "datetime" in df.columns - assert df["datetime"].isna().sum() == 0 - - @pytest.mark.parametrize( - ("peak_dt", "expected"), + "peak_dt, expected", [ ("1878-06-12", "1878-06-12"), # fully known ("1844-06-00", "1844-06-01"), # day unknown (peak_cd Bd) @@ -122,13 +108,28 @@ def test_preformat_peaks_response_drops_dateless_peaks(): """A peak with no date at all has no period to pin it to, so it cannot go on the datetime index format_response builds and is still dropped. """ - df = pd.DataFrame({"peak_dt": ["2000-03-22", None, ""], "peak_va": [1, 2, 3]}) + df = pd.DataFrame( + {"peak_dt": ["2000-03-22", np.nan, None, ""], "peak_va": [1, 2, 3, 4]} + ) df = preformat_peaks_response(df) + assert "datetime" in df.columns assert df["peak_va"].tolist() == [1] +def test_preformat_peaks_response_malformed_frame_still_raises(): + """Only an *empty* peaks frame is a legitimate empty result. A non-empty + frame with no ``peak_dt`` column is a malformed response -- a truncated or + altered RDB header -- and must stay loud rather than be returned silently + without its datetime index. + """ + df = pd.DataFrame({"peak_va": [1000]}) + + with pytest.raises(KeyError, match="peak_dt"): + format_response(df, service="peaks") + + class TestDeprecationWarnings: """Verify per-function DeprecationWarning fires with the right replacement. @@ -321,26 +322,27 @@ def test_set_metadata_info_countyCd(self, httpx_mock): class TestReadRdb: - """Tests for the NWIS-specific parse-then-format path. + """Tests for the NWIS-specific _read_rdb wrapper. The format-agnostic parser is exercised in tests/rdb_test.py; this - class pins the NWIS-specific contract — that an empty parser result - flows through format_response without crashing (issue #171), on the - plain arm via _read_rdb and on the peaks arm via format_response. + class pins the wrapper-specific contract — that an empty parser + result flows through format_response without crashing (issue #171), + on the plain arm and on the peaks arm alike. """ + NO_RESULTS_RDB = ( + "# //Output-Format: RDB\n" + "# //Response-Status: OK\n" + "# //Response-Message: No sites found matching all criteria\n" + ) + def test_no_sites_flows_through_format_response(self): """A "No sites found" response is a legitimate empty result, not an error, so callers can check ``df.empty`` rather than catching an exception. Regression for issue #171 (previously raised IndexError), which now also covers the empty-frame path through ``format_response``. """ - no_sites_rdb = ( - "# //Output-Format: RDB\n" - "# //Response-Status: OK\n" - "# //Response-Message: No sites found matching all criteria\n" - ) - df = _read_rdb(no_sites_rdb) + df = _read_rdb(self.NO_RESULTS_RDB) assert isinstance(df, pd.DataFrame) assert df.empty @@ -353,34 +355,15 @@ def test_no_peaks_flows_through_format_response(self): other service returned an empty frame (issue #171's contract). Both functions are public API, so any caller parsing a peaks RDB - reaches this. It is not dead code guarded by ``NoSitesError``: that - check in ``_querying`` fires only on a body starting "No sites/data", - which is what the live service happens to send today -- a comment-only - RDB reaches the guarded line instead. + reaches this -- it is not unreachable behind ``NoSitesError``. """ - no_peaks_rdb = ( - "# //Output-Format: RDB\n" - "# //Response-Status: OK\n" - "# //Response-Message: No sites found matching all criteria\n" - ) - # Mirror get_discharge_peaks: raw read_rdb with the NWIS dtype hints, - # then the peaks-specific format_response. - df = read_rdb(no_peaks_rdb, dtypes=_NWIS_RDB_DTYPES) + # Mirror get_discharge_peaks: raw read_rdb, then the peaks-specific + # format_response. + df = read_rdb(self.NO_RESULTS_RDB) df = format_response(df, service="peaks") assert isinstance(df, pd.DataFrame) assert df.empty - def test_malformed_peaks_frame_still_raises(self): - """Only an *empty* peaks frame is a legitimate empty result. A - non-empty frame with no ``peak_dt`` column is a malformed response -- - a truncated or altered RDB header -- and must stay loud rather than be - returned silently without its datetime index. - """ - df = pd.DataFrame({"peak_va": [1000]}) - - with pytest.raises(KeyError, match="peak_dt"): - format_response(df, service="peaks") - class TestGetRecordDispatch: """``get_record`` is a router; each service must reach its own getter. From 919b28a09a24d0e99763fec583470baf1be5ab1f Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Thu, 27 Aug 2026 09:04:11 -0500 Subject: [PATCH 6/6] fix(nwis)!: keep censored peaks as NaT instead of inventing a date Replaces the imputing fix on this branch. NWIS zero-fills the unknown part of a historical peak's date -- YYYY-MM-00 when the day is not known, YYYY-00-00 when the month is not either. Those rows were being dropped with their discharge values: 24 of 672 peaks across six sites, and disproportionately a site's largest, since the imprecisely dated peaks are the old ones. The earlier fix pinned each to the start of the known period. That put a day in the frame that the record does not claim, and because the function popped peak_dt, it also destroyed the only evidence the date was ever approximate -- the response carries no water_yr, and peak_cd holds the Bd/Bm qualifier for only 22 of those 24 rows. There is no representation that avoids this in the datetime column itself. A datetime64 value is a full instant, so pd.Timestamp("1858") IS 1858-01-01; mixed-frequency Periods fall back to object dtype and cannot form an index. The honest option is to leave datetime as NaT and keep the peak. So: parse with coerce and stop there. No normalization, no dropna, and peak_dt stays in the frame so a caller can recover the year and tell a censored date from a known one. Rows whose peak_dt is blank are kept on the same reasoning -- a missing date is not a reason to discard a discharge value. The waterservices peaks fixture has no censored dates, so its row count is unchanged; its assertion moves 240 -> 260 purely because peak_dt is retained. Restated as a shape so the two are no longer conflated. BREAKING CHANGE: peaks queries return more rows, datetime may be NaT, and peak_dt is now present in the returned frame. Co-Authored-By: Claude Opus 5 (1M context) --- NEWS.md | 3 ++- dataretrieval/nwis.py | 30 ++++++++----------------- tests/nwis_test.py | 44 +++++++++++++++++++++++-------------- tests/waterservices_test.py | 4 ++-- 4 files changed, 40 insertions(+), 41 deletions(-) diff --git a/NEWS.md b/NEWS.md index b422983fc..29ec9a628 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,4 +1,5 @@ -**08/26/2026:** **Bug fix:** `nwis.get_discharge_peaks` and `nwis.get_record(service='peaks')` silently discarded every peak whose date is only partly known. NWIS zero-fills the unknown part -- `YYYY-MM-00` when the day is not known, `YYYY-00-00` when the month is not either (the `Bd` and `Bm` `peak_cd` qualifiers) -- and neither parses as a date, so `preformat_peaks_response` coerced both to `NaT` and then dropped the row along with its discharge value. These are real peaks, and disproportionately a site's largest: site 14105700 lost 20 of its 167 peaks, among them an 1859 flood of 847,000 ft3/s, and 06934500 lost its 1844 peak of 700,000 ft3/s. Each is now dated to the start of the period that *is* known, which is how `waterdata.get_peaks()` resolves the same records -- it dates a year-only peak to 1 January and flags it `[MONTHUNKNOWN]`. A peak carrying no `peak_dt` at all is still dropped, since there is no period to place it on the datetime index `format_response` builds. **Behavior change:** peaks queries return more rows than before, and the recovered rows carry an approximate date -- read `peak_cd` for the `Bd`/`Bm` qualifier to tell them apart. +**08/27/2026:** **Bug fix:** `nwis.get_discharge_peaks` and `nwis.get_record(service='peaks')` silently discarded every peak whose date is only partly known. NWIS zero-fills the unknown part of a historical peak's date -- `YYYY-MM-00` when the day is not known, `YYYY-00-00` when the month is not either (the `Bd` and `Bm` `peak_cd` qualifiers) -- and neither parses as a date, so `preformat_peaks_response` coerced both to `NaT` and then dropped the row along with its discharge value. These are real peaks, and disproportionately a site's largest: site 14105700 lost 20 of its 167 peaks, among them an 1859 flood of 847,000 ft3/s, and 06934500 lost its 1844 peak of 700,000 ft3/s. Such a peak is now kept, with `datetime` left as `NaT`. The date is not completed into one NWIS does not have: a `datetime64` column cannot hold a partial date, so any value there would assert a day the record does not claim. **Behavior change:** peaks queries return more rows than before, and `datetime` may now be `NaT` -- a caller selecting on the datetime index will not see those peaks and should filter on `peak_dt` instead. **Behavior change:** `peak_dt` is no longer removed from the returned frame. It is the only column carrying a censored peak's year, since the peaks response has no `water_yr`, and the only dependable way to tell an unknown day from a known one -- `peak_cd` does not always carry the qualifier (22 of 24 censored dates across six sites tested). For peaks with a resolved timestamp alongside explicit `year`/`month`/`day` and a `qualifier` field, use `waterdata.get_peaks()`. + **08/26/2026:** **Bug fix:** `nwis.format_response(df, service='peaks')` and `nwis.preformat_peaks_response` raised `KeyError('peak_dt')` on an empty peaks response instead of returning an empty frame. Both are public, and every other service already treated an empty result as a legitimate empty frame rather than an error (issue #171); the peaks arm slipped through because it reformats the datetime column before the empty-frame check. Callers can now check `df.empty` rather than catching an exception. A *non-empty* frame with no `peak_dt` column is malformed rather than empty, and still raises. **08/25/2026:** Removed `dataretrieval.ogc.retry`, which only re-exported private helpers. Deprecated `dataretrieval.ogc.interruptions`; import exceptions from `dataretrieval` or `dataretrieval.interruptions` instead. The old path will be removed in a future major release, no earlier than 2027-08-25. diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index bf0e0b5db..4ef7b01a3 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -204,21 +204,6 @@ def format_response( return df.sort_index() -def _peak_datetimes(peak_dt: pd.Series) -> pd.Series: - """Parse a ``peak_dt`` column, keeping peaks whose date is partly unknown. - - NWIS writes ``YYYY-MM-00`` when the day of a historical peak is not known - and ``YYYY-00-00`` when the month is not either -- the ``Bd`` and ``Bm`` - ``peak_cd`` qualifiers. Neither parses as a date, so each is pinned to the - start of the period that *is* known, as ``waterdata.get_peaks()`` resolves - the same records. - """ - text = peak_dt.astype("string").str.strip() - text = text.str.replace(r"^(\d{4})-00-", r"\1-01-", regex=True) - text = text.str.replace(r"^(\d{4}-\d{2})-00$", r"\1-01", regex=True) - return pd.to_datetime(text, errors="coerce") - - def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: """Format the datetime column of the 'peaks' service response. @@ -236,10 +221,14 @@ def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: ----- An empty frame with no ``peak_dt`` column is returned unchanged, so an empty peaks response reaches :func:`format_response`'s empty-frame path - instead of raising ``KeyError``. Peaks whose day or month is unknown are - dated to the start of the known period rather than discarded; see - :func:`_peak_datetimes`. Rows carrying no ``peak_dt`` at all are dropped, - having no place on the datetime index :func:`format_response` builds. + instead of raising ``KeyError``. + + NWIS zero-fills the unknown part of a historical peak's date -- + ``YYYY-MM-00`` when the day is not known, ``YYYY-00-00`` when the month is + not either. Neither parses, so ``datetime`` is ``NaT`` for those peaks + rather than a date NWIS does not have. The peak is kept regardless, and + ``peak_dt`` is left in the frame: the response carries no ``water_yr``, so + it is the only column holding a censored peak's year. """ if df.empty and "peak_dt" not in df.columns: @@ -247,8 +236,7 @@ def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: # still raise. return df - df["datetime"] = _peak_datetimes(df.pop("peak_dt")) - df.dropna(subset=["datetime"], inplace=True) + df["datetime"] = pd.to_datetime(df["peak_dt"], errors="coerce") return df diff --git a/tests/nwis_test.py b/tests/nwis_test.py index ef725509a..49ea537a7 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -85,37 +85,47 @@ def test_iv_service_answer(httpx_mock): "peak_dt, expected", [ ("1878-06-12", "1878-06-12"), # fully known - ("1844-06-00", "1844-06-01"), # day unknown (peak_cd Bd) - ("1858-00-00", "1858-01-01"), # month unknown (peak_cd Bm) + ("1844-06-00", None), # day unknown (peak_cd Bd) + ("1858-00-00", None), # month unknown (peak_cd Bm) + ("", None), # no date at all + (np.nan, None), ], ) -def test_preformat_peaks_response_keeps_partial_dates(peak_dt, expected): - """NWIS zero-fills the unknown part of a historical peak's date, and those - are real peaks -- often a site's largest. They must be pinned to the start - of the known period, not coerced to NaT and dropped. ``waterdata.get_peaks`` - resolves the same records the same way. +def test_preformat_peaks_response_keeps_every_peak(peak_dt, expected): + """A peak is never dropped for want of a parseable date. + + NWIS zero-fills the unknown part of a historical peak's date -- + ``YYYY-MM-00`` when the day is not known, ``YYYY-00-00`` when the month is + not either (the ``Bd`` and ``Bm`` ``peak_cd`` qualifiers). Those are real + peaks, often a site's largest, and dropping them loses the discharge value + with the date. A date NWIS only partly knows stays ``NaT`` rather than + being completed into one it does not have. """ df = pd.DataFrame({"peak_dt": [peak_dt], "peak_va": [563000]}) df = preformat_peaks_response(df) - assert len(df) == 1, f"{peak_dt} was dropped" - assert df["datetime"].iloc[0] == pd.Timestamp(expected) + assert len(df) == 1, f"{peak_dt!r} was dropped" assert df["peak_va"].iloc[0] == 563000 + if expected is None: + assert pd.isna(df["datetime"].iloc[0]) + else: + assert df["datetime"].iloc[0] == pd.Timestamp(expected) -def test_preformat_peaks_response_drops_dateless_peaks(): - """A peak with no date at all has no period to pin it to, so it cannot go - on the datetime index format_response builds and is still dropped. +def test_preformat_peaks_response_preserves_peak_dt(): + """``peak_dt`` must survive the reformat. + + The peaks response carries no ``water_yr``, so ``peak_dt`` is the only + column holding the year of a censored peak -- and the only way a caller can + tell an unknown day from a known one, since ``peak_cd`` does not always + carry the qualifier. """ - df = pd.DataFrame( - {"peak_dt": ["2000-03-22", np.nan, None, ""], "peak_va": [1, 2, 3, 4]} - ) + df = pd.DataFrame({"peak_dt": ["1858-00-00"], "peak_va": [563000]}) df = preformat_peaks_response(df) - assert "datetime" in df.columns - assert df["peak_va"].tolist() == [1] + assert df["peak_dt"].iloc[0] == "1858-00-00" def test_preformat_peaks_response_malformed_frame_still_raises(): diff --git a/tests/waterservices_test.py b/tests/waterservices_test.py index 291c64a15..0fda62d84 100644 --- a/tests/waterservices_test.py +++ b/tests/waterservices_test.py @@ -242,7 +242,7 @@ def test_get_discharge_peaks(httpx_mock): if not isinstance(df, DataFrame): raise TypeError(f"{type(df)} is not DataFrame base class type") - assert df.size == 240 + assert df.shape == (20, 13) assert_metadata(httpx_mock, request_url, md, site, None, format) @@ -267,7 +267,7 @@ def test_get_discharge_peaks_sites_value_types(httpx_mock, site_input_type_list) if not isinstance(df, DataFrame): raise TypeError(f"{type(df)} is not DataFrame base class type") - assert df.size == 240 + assert df.shape == (20, 13) def test_get_ratings_validation():