diff --git a/NEWS.md b/NEWS.md index 6dfb3ba9..29ec9a62 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,7 @@ +**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. **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 44d74828..4ef7b01a 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -217,9 +217,26 @@ 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 an + empty peaks response reaches :func:`format_response`'s empty-frame path + 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. + """ - df["datetime"] = pd.to_datetime(df.pop("peak_dt"), errors="coerce") - df.dropna(subset=["datetime"], inplace=True) + if df.empty and "peak_dt" not in df.columns: + # A non-empty frame missing peak_dt is malformed, not empty, and must + # still raise. + return df + + 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 9c424fa0..49ea537a 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -13,6 +13,7 @@ from dataretrieval.nwis import ( NWIS_Metadata, _read_rdb, + format_response, get_discharge_measurements, get_gwlevels, get_iv, @@ -22,6 +23,7 @@ get_water_use, preformat_peaks_response, ) +from dataretrieval.rdb import read_rdb START_DATE = "2018-01-24" END_DATE = "2018-01-25" @@ -79,17 +81,63 @@ 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 +@pytest.mark.parametrize( + "peak_dt, expected", + [ + ("1878-06-12", "1878-06-12"), # fully known + ("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_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!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_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": ["1858-00-00"], "peak_va": [563000]}) + df = preformat_peaks_response(df) - # assertions - assert "datetime" in df.columns - assert df["datetime"].isna().sum() == 0 + + assert df["peak_dt"].iloc[0] == "1858-00-00" + + +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: @@ -288,21 +336,41 @@ class TestReadRdb: 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). + 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 + + def test_no_peaks_flows_through_format_response(self): + """``format_response(service="peaks")`` must tolerate an empty frame. + + 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 API, so any caller parsing a peaks RDB + reaches this -- it is not unreachable behind ``NoSitesError``. + """ + # 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 diff --git a/tests/waterservices_test.py b/tests/waterservices_test.py index 291c64a1..0fda62d8 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():