feat: rolling aggregations, and reject irregular period grids - #12
Open
viniciusoike wants to merge 2 commits into
Open
feat: rolling aggregations, and reject irregular period grids#12viniciusoike wants to merge 2 commits into
viniciusoike wants to merge 2 commits into
Conversation
Adds a third API family alongside trends and decomposition, covering rolling and year-to-date aggregations: augment_rolling() for data frames and roll_series() for ts/xts/zoo, mirroring the augment_trends() / extract_trends() pairing. Six statistics: sum, chain, mean, sd, min, max. Columns are prefixed roll_ rather than trend_ because these are aggregations of the series, not estimates of its trend. A rolling sum is not in the units of the series, so the statistics live in a separate registry (.rolling_info()) and can never reach detrend_series(), which would subtract them from the series. A test asserts the two registries stay disjoint. The value over calling RcppRoll directly is concentrated in two places: - stats = "chain" compounds rates as prod(1 + r) - 1, the correct accumulation for a series that is already a rate of change. Summing monthly inflation only approximates the 12-month figure. Since a 100x scale error here is otherwise silent, percent is explicit and a heuristic warns when the declared scale looks wrong. - window = "ytd" gives an expanding accumulation that resets each January or Q1, for any of the six statistics. align defaults to "right", the convention for accumulated economic indicators, rather than the centred default used for trends. Missing values keep their calendar position via a new .df_to_ts_preserve_na(), so windows stay aligned with the dates and na_rm decides how each window treats them. .trends_to_df() gains a prefix argument so the naming and merge logic is shared. 160 new assertions across 50 test blocks. R CMD check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
augment_trends(), decompose_series(), deseason_series(), and detrend_series() returned plausible-looking but wrong output for any series with a gap in the middle. .df_to_ts_internal() drops incomplete cases, then rebuilds the ts from `start` assuming contiguous periods. A missing period therefore shifts every later observation one slot earlier, and time() lies from that point on. Results are merged back by date, so they land on the wrong rows, and the series loses its final period. In a 36-month series with one missing month, 23 of the 24 rows after the gap were wrong and the last trend value moved from 1984-01 to 1983-12. The worst case is decompose_series(), whose documented identity value = trend + seasonal + remainder held on only 19 of 59 rows, with a max deviation of 51,944. The comment claiming the date-keyed merge was robust to NA-induced length mismatches was wrong: it is robust to the merge erroring, not to values being attached to the wrong dates. Two situations triggered this and neither warned. A row whose value was NA, and a period absent from the data entirely -- the latter with no NA anywhere in the input. Both now abort, naming the missing periods and distinguishing the two causes so the message points at the right fix. Duplicated periods, which corrupt the series the same way, are rejected too. Aborting rather than repairing is deliberate. Fitting through a gap needs a per-method policy for missing values, and 6 of the 20 trend methods cannot produce a result from a series containing NA at all: bk, stl, spline and hamilton error, while hp and cf return all-NA. That is a larger change than a pre-submission fix should carry. Leading and trailing missing values cannot open an interior gap and continue to work unchanged. Frequencies with no exact calendar period (weekly, daily) are not checked. No bundled dataset is affected: all 12 have complete grids, and retail_volume's 96 NAs are leading. Three tests that asserted the old tolerant behaviour for interior NAs have been updated to assert the error, plus coverage for the identity holding on complete data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Adds the
augment_rolling()/roll_series()family, and fixes a silent data-corruption bug found while building it.1. Rolling aggregations (
41fbb5a)A third API family alongside trends and decomposition:
augment_rolling()for data frames,roll_series()for ts/xts/zoo, mirroring theaugment_trends()/extract_trends()pairing.Six statistics —
sum,chain,mean,sd,min,max— withwindowaccepting a scalar, a vector (c(3, 6, 12)→ one column each), or"ytd". Columns are namedroll_{stat}_{window}.Why this isn't just an RcppRoll wrapper. The value is concentrated in two places:
stats = "chain"compounds rates asprod(1 + r) - 1, the correct accumulation for a series that is already a rate of change. Summing monthly inflation only approximates the 12-month figure, and that approximation is routinely written by hand and routinely gotten wrong. Because a 100x scale error here is otherwise silent,percentis explicit and a heuristic warns when the declared scale looks implausible.window = "ytd"gives an expanding accumulation resetting each January or Q1, for any of the six statistics.aligndefaults to"right"— the convention for accumulated economic indicators — rather than the centred default used for trends.Kept out of the trend registry, deliberately. A rolling sum is not in the units of the series, so if it reached
detrend_series()the result would be-11 × y. The statistics live in a separate registry (.rolling_info()), and a test asserts the two stay disjoint.roll_series(x, "mean", window = k, align = "right")is exactlyextract_trends(x, "ma", window = k, align = "right"); the rolling sum isktimes that. The rolling family is for when the accumulated quantity is itself the number of interest.2. Irregular period grids now error (
0df4db6)augment_trends(),decompose_series(),deseason_series(), anddetrend_series()returned plausible-looking but wrong output for any series with a gap in the middle..df_to_ts_internal()drops incomplete cases, then rebuilds thetsfromstartassuming contiguous periods — so a missing period shifts every later observation one slot earlier andtime()lies from there on. Results are merged back by date, land on the wrong rows, and the series loses its final period.23 of 24 post-gap rows wrong. Worst case is
decompose_series(), whose documentedvalue = trend + seasonal + remainderidentity held on 19 of 59 rows, max deviation 51,944.Two triggers, neither of which warned: a row whose value is
NA, and a period absent from the data entirely — the latter with noNAanywhere in the input. Both now abort, naming the missing periods and distinguishing the two causes. Duplicated periods are rejected too.Why abort rather than repair. Fitting through a gap needs a per-method policy for missing values, and 6 of the 20 trend methods cannot produce a result from a series containing
NA:bk,stl,spline,hamiltonerror;hpandcfreturn all-NA. That is a larger change than a pre-submission fix should carry, and is flagged in NEWS as future work.Leading and trailing missing values cannot open an interior gap and are unaffected. Weekly/daily frequencies have no exact calendar period and are not checked.
Compatibility
No bundled dataset is affected — all 12 have complete grids, and
retail_volume's 96 NAs are leading. Package examples, vignettes, and pre-existing tests are unchanged in behaviour.Three tests that asserted the old tolerant behaviour for interior NAs now assert the error instead. Anyone currently passing gapped data was getting misdated results and will now get an error telling them which periods to fill.
Verification
R CMD check: 0 errors, 0 warnings, 0 notesNote on versioning
NEWS entries are under
# trendseries (development version);DESCRIPTIONis left at 1.4.0 since that submission is in flight. Bumping to 1.4.0.9000 is your call.Still outstanding
No vignette for the rolling family yet — noted in
CLAUDE.md.🤖 Generated with Claude Code