From 68674ebab064a2b7a17891ad3cb311ca331e4774 Mon Sep 17 00:00:00 2001 From: notskynet-bot Date: Tue, 17 Feb 2026 09:06:19 +0700 Subject: [PATCH 1/3] Phase 2.1: incidence rolling/cumulative + docs --- PR3_CI2_REPLY.md | 7 +++++ PR3_CI_REPLY.md | 8 +++++ PR3_FIX_MMwR_REPLY.md | 12 ++++++++ PR3_MENTION_REPLY.md | 13 ++++++++ PR3_REPLY.md | 8 +++++ PR4_REPLY.md | 9 ++++++ PR_PHASE2_1.md | 29 ++++++++++++++++++ README.md | 4 ++- epydem/incidence.py | 32 ++++++++++++++++++-- tests/test_incidence_phase2_1.py | 52 ++++++++++++++++++++++++++++++++ 10 files changed, 171 insertions(+), 3 deletions(-) create mode 100644 PR3_CI2_REPLY.md create mode 100644 PR3_CI_REPLY.md create mode 100644 PR3_FIX_MMwR_REPLY.md create mode 100644 PR3_MENTION_REPLY.md create mode 100644 PR3_REPLY.md create mode 100644 PR4_REPLY.md create mode 100644 PR_PHASE2_1.md create mode 100644 tests/test_incidence_phase2_1.py diff --git a/PR3_CI2_REPLY.md b/PR3_CI2_REPLY.md new file mode 100644 index 0000000..2d07c22 --- /dev/null +++ b/PR3_CI2_REPLY.md @@ -0,0 +1,7 @@ +You're right — CI was still failing (ruff) due to import/style issues in `epydem/incidence.py`. + +Fixes pushed: +- Use `Sequence` from `collections.abc` (UP035). +- Sort/organize imports and remove inner imports (I001). + +Commit: a4e355c diff --git a/PR3_CI_REPLY.md b/PR3_CI_REPLY.md new file mode 100644 index 0000000..6097606 --- /dev/null +++ b/PR3_CI_REPLY.md @@ -0,0 +1,8 @@ +Good call — I agree. + +I fixed the CI failures on PR#3 before asking you to review: +- Removed unused imports in `epydem/epiweek.py` (ruff F401). +- Modernized typing in `epydem/time.py` (avoid deprecated `typing.Tuple/Union`, use `tuple[...]` and `X | Y`). +- Wrapped a long test line to satisfy line-length. + +Commit: 485f610 diff --git a/PR3_FIX_MMwR_REPLY.md b/PR3_FIX_MMwR_REPLY.md new file mode 100644 index 0000000..dc03476 --- /dev/null +++ b/PR3_FIX_MMwR_REPLY.md @@ -0,0 +1,12 @@ +Root cause found for the remaining CI failures: our CDC/MMWR year logic was wrong at the year boundary. + +Spec nuance: +- 2023-12-31 is the start of 2024 week 1 (week 1 contains Jan 4). +- Our previous implementation assigned it to (2023, 53), which broke weekly fill_missing ranges and the incidence test. + +Fix: +- Updated `mmwr_week()` to choose the unique epi-year such that: + `week1_start(year) <= d < week1_start(year + 1)` +- Updated epiweek boundary tests accordingly. + +Commit: 607b258 diff --git a/PR3_MENTION_REPLY.md b/PR3_MENTION_REPLY.md new file mode 100644 index 0000000..994887d --- /dev/null +++ b/PR3_MENTION_REPLY.md @@ -0,0 +1,13 @@ +Thanks — and fair question. + +Why it took longer than it should have: +1) **I optimized the wrong thing first.** I kept iterating on ruff/style (I001, UP035) and didn’t re-check the *core semantic assumption* (MMWR year boundaries) early enough. +2) **I lacked a fast local feedback loop.** On the OpenClaw host we don’t have `pip`/deps installed, so I relied on GitHub Actions runs to validate. That made each iteration slower. +3) **MMWR has a tricky year boundary nuance.** The key miss was that `2023-12-31` is actually the start of `2024-W01` (week 1 contains Jan 4). My first implementation incorrectly labeled it as `(2023, 53)`, which then cascaded into the incidence fill_missing range and tests. + +What I’m changing going forward (to avoid repeats): +- **Always ack mentions immediately** with an ETA, even before the fix lands. +- When CI fails, **classify failures**: (a) style/lint vs (b) tests/semantics. For (b), I’ll immediately add/verify boundary examples (like year transitions) *before* doing more lint churn. +- I won’t ping you for review until **all checks are green**. + +Appreciate the push — it helped tighten the workflow. diff --git a/PR3_REPLY.md b/PR3_REPLY.md new file mode 100644 index 0000000..44fa49f --- /dev/null +++ b/PR3_REPLY.md @@ -0,0 +1,8 @@ +Thanks — agreed on DX. I updated `fill_missing=True` to fill **per stratum** (not just overall): + +- For `freq="D"`: build full (stratum × date) grid between min/max date, then left-join counts and fill 0. +- For `freq="W-MMWR"`: build full (stratum × (epi_year, epi_week)) grid between min/max observed week, then fill 0. + +Tests updated to cover a stratum with a missing week now yielding 0. + +Commit: 5ae982e diff --git a/PR4_REPLY.md b/PR4_REPLY.md new file mode 100644 index 0000000..76ffb69 --- /dev/null +++ b/PR4_REPLY.md @@ -0,0 +1,9 @@ +Thanks — agreed. I consolidated the workflows so we only have **one** CI. + +What changed in this PR: +- Removed the redundant `.github/workflows/test.yml`. +- Kept `.github/workflows/ci.yml` as the single source of truth. +- CI still tests Python 3.10–3.12 (matches `requires-python >= 3.10`). +- Added a quick import smoke test to CI. + +Commit: 1f20664 diff --git a/PR_PHASE2_1.md b/PR_PHASE2_1.md new file mode 100644 index 0000000..b54aceb --- /dev/null +++ b/PR_PHASE2_1.md @@ -0,0 +1,29 @@ +Phase 2.1: incidence DX improvements (docs + rolling/cumulative) + +Key points +- Add optional `cumulative=True` for wide output. +- Add optional `rolling=` for wide output with `rolling_kind` (sum/mean). +- Expand README examples for incidence. + +Why this implementation +- These options are common in epi workflows (cumulative incidence, smoothed curves) and improve usability without changing the core incidence semantics. +- Implemented at the wide-output layer so behavior is consistent across strata columns. + +Multi-role debate (differences, not consensus) + +Role A — pragmatic developer +- 👍 Likes: small API surface change, easy to test, minimal code. +- ⚠️ Concern: rolling/cumulative after pivot means we assume the index ordering fully represents time (true for date and (epi_year, epi_week) sorted). + +Role B — architecture +- 👍 Likes: keeps incidence core as counts; transformations are optional flags. +- ⚠️ Concern: feature creep: might prefer a separate `transform_incidence()` pipeline later. + +Role C — developer user (DX) +- 👍 Likes: fewer steps in notebooks; one-liners for rolling/cumulative. +- ⚠️ Concern: wants more control (e.g., min_periods, centered rolling, cumulative per-calendar-year vs epi-year). + +Points of divergence to revisit later +1) Whether these transforms belong inside `incidence()` long-term. +2) Add `min_periods`, `center`, and `cumulative_by` options. +3) Apply transforms to `output="long"` as well. diff --git a/README.md b/README.md index 59597c4..32ba323 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,9 @@ print(week_only) # Incidence (line list -> counts) # df = pandas.DataFrame({"onset_date": [...], "sex": [...]}) -# weekly = epydem.incidence(df, date_col="onset_date", freq="W-MMWR", by=["sex"]) +# weekly = epydem.incidence(df, date_col="onset_date", freq="W-MMWR", by=["sex"], fill_missing=True) +# weekly_rolling2 = epydem.incidence(df, date_col="onset_date", freq="W-MMWR", by=["sex"], rolling=2) +# weekly_cum = epydem.incidence(df, date_col="onset_date", freq="W-MMWR", by=["sex"], cumulative=True) ``` ## Roadmap (high level) diff --git a/epydem/incidence.py b/epydem/incidence.py index da37631..ac8160a 100644 --- a/epydem/incidence.py +++ b/epydem/incidence.py @@ -29,6 +29,9 @@ def incidence( count_col: str = "cases", output: OutputFormat = "wide", fill_missing: bool = True, + cumulative: bool = False, + rolling: int | None = None, + rolling_kind: Literal["sum", "mean"] = "sum", ) -> pd.DataFrame: """Compute incidence counts from a line list. @@ -44,6 +47,9 @@ def incidence( - "wide" (default): pivot table style (DX-friendly) - "long": tidy long-form table fill_missing: If True, fill missing dates/weeks with 0 counts. + cumulative: If True, return cumulative sum over time (per column in wide output). + rolling: If set, compute a rolling window over time (per column in wide output). + rolling_kind: "sum" or "mean" for the rolling aggregation. Returns: DataFrame in the requested output format. @@ -104,7 +110,18 @@ def incidence( else: wide = long.set_index("date")[[count_col]] - return wide.sort_index() + wide = wide.sort_index() + + if rolling is not None: + if rolling_kind == "sum": + wide = wide.rolling(window=rolling, min_periods=1).sum() + else: + wide = wide.rolling(window=rolling, min_periods=1).mean() + + if cumulative: + wide = wide.cumsum() + + return wide if freq == "W-MMWR": # Compute (epi_year, epi_week) for unique dates, then map back for performance. @@ -168,6 +185,17 @@ def incidence( else: wide = long.set_index(["epi_year", "epi_week"])[[count_col]] - return wide.sort_index() + wide = wide.sort_index() + + if rolling is not None: + if rolling_kind == "sum": + wide = wide.rolling(window=rolling, min_periods=1).sum() + else: + wide = wide.rolling(window=rolling, min_periods=1).mean() + + if cumulative: + wide = wide.cumsum() + + return wide raise ValueError(f"Unknown freq: {freq}") diff --git a/tests/test_incidence_phase2_1.py b/tests/test_incidence_phase2_1.py new file mode 100644 index 0000000..78bca2f --- /dev/null +++ b/tests/test_incidence_phase2_1.py @@ -0,0 +1,52 @@ +from __future__ import annotations + +import pandas as pd + +import epydem + + +def test_incidence_weekly_rolling_and_cumulative(): + df = pd.DataFrame( + { + "onset_date": [ + "2024-01-01", # (2024,1) + "2024-01-08", # (2024,2) + "2024-01-08", + ], + "province": ["A", "A", "A"], + } + ) + + base = epydem.incidence( + df, + date_col="onset_date", + freq="W-MMWR", + by=["province"], + fill_missing=True, + ) + # week1=1, week2=2 + assert base.loc[(2024, 1), "A"] == 1 + assert base.loc[(2024, 2), "A"] == 2 + + roll2 = epydem.incidence( + df, + date_col="onset_date", + freq="W-MMWR", + by=["province"], + fill_missing=True, + rolling=2, + rolling_kind="sum", + ) + assert roll2.loc[(2024, 1), "A"] == 1 + assert roll2.loc[(2024, 2), "A"] == 3 + + cum = epydem.incidence( + df, + date_col="onset_date", + freq="W-MMWR", + by=["province"], + fill_missing=True, + cumulative=True, + ) + assert cum.loc[(2024, 1), "A"] == 1 + assert cum.loc[(2024, 2), "A"] == 3 From e84e3cdb7157f615c41071bc316ab63d2a69c883 Mon Sep 17 00:00:00 2001 From: notskynet-bot Date: Tue, 17 Feb 2026 09:06:26 +0700 Subject: [PATCH 2/3] Chore: remove local PR reply scratch files --- PR3_CI2_REPLY.md | 7 ------- PR3_CI_REPLY.md | 8 -------- PR3_FIX_MMwR_REPLY.md | 12 ------------ PR3_MENTION_REPLY.md | 13 ------------- PR3_REPLY.md | 8 -------- PR4_REPLY.md | 9 --------- 6 files changed, 57 deletions(-) delete mode 100644 PR3_CI2_REPLY.md delete mode 100644 PR3_CI_REPLY.md delete mode 100644 PR3_FIX_MMwR_REPLY.md delete mode 100644 PR3_MENTION_REPLY.md delete mode 100644 PR3_REPLY.md delete mode 100644 PR4_REPLY.md diff --git a/PR3_CI2_REPLY.md b/PR3_CI2_REPLY.md deleted file mode 100644 index 2d07c22..0000000 --- a/PR3_CI2_REPLY.md +++ /dev/null @@ -1,7 +0,0 @@ -You're right — CI was still failing (ruff) due to import/style issues in `epydem/incidence.py`. - -Fixes pushed: -- Use `Sequence` from `collections.abc` (UP035). -- Sort/organize imports and remove inner imports (I001). - -Commit: a4e355c diff --git a/PR3_CI_REPLY.md b/PR3_CI_REPLY.md deleted file mode 100644 index 6097606..0000000 --- a/PR3_CI_REPLY.md +++ /dev/null @@ -1,8 +0,0 @@ -Good call — I agree. - -I fixed the CI failures on PR#3 before asking you to review: -- Removed unused imports in `epydem/epiweek.py` (ruff F401). -- Modernized typing in `epydem/time.py` (avoid deprecated `typing.Tuple/Union`, use `tuple[...]` and `X | Y`). -- Wrapped a long test line to satisfy line-length. - -Commit: 485f610 diff --git a/PR3_FIX_MMwR_REPLY.md b/PR3_FIX_MMwR_REPLY.md deleted file mode 100644 index dc03476..0000000 --- a/PR3_FIX_MMwR_REPLY.md +++ /dev/null @@ -1,12 +0,0 @@ -Root cause found for the remaining CI failures: our CDC/MMWR year logic was wrong at the year boundary. - -Spec nuance: -- 2023-12-31 is the start of 2024 week 1 (week 1 contains Jan 4). -- Our previous implementation assigned it to (2023, 53), which broke weekly fill_missing ranges and the incidence test. - -Fix: -- Updated `mmwr_week()` to choose the unique epi-year such that: - `week1_start(year) <= d < week1_start(year + 1)` -- Updated epiweek boundary tests accordingly. - -Commit: 607b258 diff --git a/PR3_MENTION_REPLY.md b/PR3_MENTION_REPLY.md deleted file mode 100644 index 994887d..0000000 --- a/PR3_MENTION_REPLY.md +++ /dev/null @@ -1,13 +0,0 @@ -Thanks — and fair question. - -Why it took longer than it should have: -1) **I optimized the wrong thing first.** I kept iterating on ruff/style (I001, UP035) and didn’t re-check the *core semantic assumption* (MMWR year boundaries) early enough. -2) **I lacked a fast local feedback loop.** On the OpenClaw host we don’t have `pip`/deps installed, so I relied on GitHub Actions runs to validate. That made each iteration slower. -3) **MMWR has a tricky year boundary nuance.** The key miss was that `2023-12-31` is actually the start of `2024-W01` (week 1 contains Jan 4). My first implementation incorrectly labeled it as `(2023, 53)`, which then cascaded into the incidence fill_missing range and tests. - -What I’m changing going forward (to avoid repeats): -- **Always ack mentions immediately** with an ETA, even before the fix lands. -- When CI fails, **classify failures**: (a) style/lint vs (b) tests/semantics. For (b), I’ll immediately add/verify boundary examples (like year transitions) *before* doing more lint churn. -- I won’t ping you for review until **all checks are green**. - -Appreciate the push — it helped tighten the workflow. diff --git a/PR3_REPLY.md b/PR3_REPLY.md deleted file mode 100644 index 44fa49f..0000000 --- a/PR3_REPLY.md +++ /dev/null @@ -1,8 +0,0 @@ -Thanks — agreed on DX. I updated `fill_missing=True` to fill **per stratum** (not just overall): - -- For `freq="D"`: build full (stratum × date) grid between min/max date, then left-join counts and fill 0. -- For `freq="W-MMWR"`: build full (stratum × (epi_year, epi_week)) grid between min/max observed week, then fill 0. - -Tests updated to cover a stratum with a missing week now yielding 0. - -Commit: 5ae982e diff --git a/PR4_REPLY.md b/PR4_REPLY.md deleted file mode 100644 index 76ffb69..0000000 --- a/PR4_REPLY.md +++ /dev/null @@ -1,9 +0,0 @@ -Thanks — agreed. I consolidated the workflows so we only have **one** CI. - -What changed in this PR: -- Removed the redundant `.github/workflows/test.yml`. -- Kept `.github/workflows/ci.yml` as the single source of truth. -- CI still tests Python 3.10–3.12 (matches `requires-python >= 3.10`). -- Added a quick import smoke test to CI. - -Commit: 1f20664 From f5b4e126f6ee64dea6561dfe3e21550c60d8a823 Mon Sep 17 00:00:00 2001 From: notskynet-bot Date: Tue, 17 Feb 2026 14:17:58 +0700 Subject: [PATCH 3/3] Refactor: separate transform_incidence() from incidence() --- README.md | 4 +- epydem/__init__.py | 2 + epydem/incidence.py | 32 +--------- epydem/transform.py | 106 +++++++++++++++++++++++++++++++ tests/test_incidence_phase2_1.py | 16 ++--- 5 files changed, 116 insertions(+), 44 deletions(-) create mode 100644 epydem/transform.py diff --git a/README.md b/README.md index 32ba323..2732ba6 100644 --- a/README.md +++ b/README.md @@ -34,8 +34,8 @@ print(week_only) # Incidence (line list -> counts) # df = pandas.DataFrame({"onset_date": [...], "sex": [...]}) # weekly = epydem.incidence(df, date_col="onset_date", freq="W-MMWR", by=["sex"], fill_missing=True) -# weekly_rolling2 = epydem.incidence(df, date_col="onset_date", freq="W-MMWR", by=["sex"], rolling=2) -# weekly_cum = epydem.incidence(df, date_col="onset_date", freq="W-MMWR", by=["sex"], cumulative=True) +# weekly_rolling2 = epydem.transform_incidence(weekly, rolling=2) +# weekly_cum = epydem.transform_incidence(weekly, cumulative=True) ``` ## Roadmap (high level) diff --git a/epydem/__init__.py b/epydem/__init__.py index aa7026f..09ebc34 100644 --- a/epydem/__init__.py +++ b/epydem/__init__.py @@ -3,6 +3,7 @@ from .epiweek import calculate from .incidence import incidence from .time import epiweek, epiweek_number, mmwr_week, mmwr_week1_start, parse_ymd +from .transform import transform_incidence __all__ = [ "calculate", @@ -12,4 +13,5 @@ "mmwr_week1_start", "parse_ymd", "incidence", + "transform_incidence", ] diff --git a/epydem/incidence.py b/epydem/incidence.py index ac8160a..da37631 100644 --- a/epydem/incidence.py +++ b/epydem/incidence.py @@ -29,9 +29,6 @@ def incidence( count_col: str = "cases", output: OutputFormat = "wide", fill_missing: bool = True, - cumulative: bool = False, - rolling: int | None = None, - rolling_kind: Literal["sum", "mean"] = "sum", ) -> pd.DataFrame: """Compute incidence counts from a line list. @@ -47,9 +44,6 @@ def incidence( - "wide" (default): pivot table style (DX-friendly) - "long": tidy long-form table fill_missing: If True, fill missing dates/weeks with 0 counts. - cumulative: If True, return cumulative sum over time (per column in wide output). - rolling: If set, compute a rolling window over time (per column in wide output). - rolling_kind: "sum" or "mean" for the rolling aggregation. Returns: DataFrame in the requested output format. @@ -110,18 +104,7 @@ def incidence( else: wide = long.set_index("date")[[count_col]] - wide = wide.sort_index() - - if rolling is not None: - if rolling_kind == "sum": - wide = wide.rolling(window=rolling, min_periods=1).sum() - else: - wide = wide.rolling(window=rolling, min_periods=1).mean() - - if cumulative: - wide = wide.cumsum() - - return wide + return wide.sort_index() if freq == "W-MMWR": # Compute (epi_year, epi_week) for unique dates, then map back for performance. @@ -185,17 +168,6 @@ def incidence( else: wide = long.set_index(["epi_year", "epi_week"])[[count_col]] - wide = wide.sort_index() - - if rolling is not None: - if rolling_kind == "sum": - wide = wide.rolling(window=rolling, min_periods=1).sum() - else: - wide = wide.rolling(window=rolling, min_periods=1).mean() - - if cumulative: - wide = wide.cumsum() - - return wide + return wide.sort_index() raise ValueError(f"Unknown freq: {freq}") diff --git a/epydem/transform.py b/epydem/transform.py new file mode 100644 index 0000000..325c763 --- /dev/null +++ b/epydem/transform.py @@ -0,0 +1,106 @@ +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import dataclass +from typing import Literal + +import pandas as pd + +RollingKind = Literal["sum", "mean"] + + +@dataclass(frozen=True) +class TransformSpec: + rolling: int | None = None + rolling_kind: RollingKind = "sum" + min_periods: int = 1 + center: bool = False + cumulative: bool = False + + +def transform_incidence( + data: pd.DataFrame, + *, + rolling: int | None = None, + rolling_kind: RollingKind = "sum", + min_periods: int = 1, + center: bool = False, + cumulative: bool = False, + time_cols: Sequence[str] | None = None, +) -> pd.DataFrame: + """Apply time-series transforms to incidence outputs. + + This is intentionally separate from `incidence()` to keep `incidence()` as a + stable counts primitive. + + Supported inputs: + - Wide incidence output: index is time (e.g. date or (epi_year, epi_week)). + - Long incidence output: requires `time_cols` (or inferred), and transforms + are applied per non-time column group. + + Args: + data: Output from `epydem.incidence(...)`. + rolling: Rolling window size. + rolling_kind: "sum" or "mean". + min_periods: Passed to pandas rolling. + center: Passed to pandas rolling. + cumulative: If True, apply cumulative sum. + time_cols: For long-form data, the time columns. If None, we try to infer + ("date") or ("epi_year", "epi_week"). + + Returns: + Transformed DataFrame with the same shape/schema as input. + """ + + if rolling is None and not cumulative: + return data + + # Wide: DataFrameIndex is time. + if time_cols is None and ("date" not in data.columns) and ("epi_year" not in data.columns): + wide = data.sort_index() + if rolling is not None: + r = wide.rolling(window=rolling, min_periods=min_periods, center=center) + wide = r.sum() if rolling_kind == "sum" else r.mean() + if cumulative: + wide = wide.cumsum() + return wide + + # Long: operate on a value column per group. + df = data.copy() + + if time_cols is None: + if "date" in df.columns: + time_cols = ["date"] + elif ("epi_year" in df.columns) and ("epi_week" in df.columns): + time_cols = ["epi_year", "epi_week"] + else: + raise ValueError("Cannot infer time_cols for long-form incidence") + + value_cols = [c for c in df.columns if c not in set(time_cols)] + if len(value_cols) != 1: + raise ValueError( + "Long-form incidence must have exactly one value column (e.g., 'cases'). " + f"Got {value_cols}" + ) + value_col = value_cols[0] + + group_cols = [c for c in df.columns if c not in set(time_cols + [value_col])] + + df = df.sort_values(list(time_cols)) + + def _apply(group: pd.DataFrame) -> pd.DataFrame: + s = group[value_col] + if rolling is not None: + r = s.rolling(window=rolling, min_periods=min_periods, center=center) + s = r.sum() if rolling_kind == "sum" else r.mean() + if cumulative: + s = s.cumsum() + group[value_col] = s + return group + + if group_cols: + out = df.groupby(group_cols, dropna=False, sort=False, group_keys=False).apply(_apply) + else: + out = _apply(df) + + return out diff --git a/tests/test_incidence_phase2_1.py b/tests/test_incidence_phase2_1.py index 78bca2f..99ea2cd 100644 --- a/tests/test_incidence_phase2_1.py +++ b/tests/test_incidence_phase2_1.py @@ -28,24 +28,16 @@ def test_incidence_weekly_rolling_and_cumulative(): assert base.loc[(2024, 1), "A"] == 1 assert base.loc[(2024, 2), "A"] == 2 - roll2 = epydem.incidence( - df, - date_col="onset_date", - freq="W-MMWR", - by=["province"], - fill_missing=True, + roll2 = epydem.transform_incidence( + base, rolling=2, rolling_kind="sum", ) assert roll2.loc[(2024, 1), "A"] == 1 assert roll2.loc[(2024, 2), "A"] == 3 - cum = epydem.incidence( - df, - date_col="onset_date", - freq="W-MMWR", - by=["province"], - fill_missing=True, + cum = epydem.transform_incidence( + base, cumulative=True, ) assert cum.loc[(2024, 1), "A"] == 1