Skip to content

Implement subset of split-apply-combine pattern#396

Open
ph-kev wants to merge 2 commits into
mainfrom
kp/split-apply-merge
Open

Implement subset of split-apply-combine pattern#396
ph-kev wants to merge 2 commits into
mainfrom
kp/split-apply-merge

Conversation

@ph-kev
Copy link
Copy Markdown
Member

@ph-kev ph-kev commented May 15, 2026

closes #279

This PR implements a subset of the split-apply-combine pattern which are supporting reductions, single group split, and seasonal splits. This allows the users to be more expressive with the kind of reductions that they can do. The implementation of the split-apply-combine pattern also keep the dimension that is reduced over which can be helpful for calibration (e.g. certain calibration functionality such as ObservationRecipe requires a time dimension).

Here's a list of concerns that I have for this PR:

  1. I wasn't sure if this should be in a module or not. I decided against it since putting the functionality in a module isn't a breaking change as long as I export the fucntion.
  2. I couldn't get the docs for the struct behaving as a function working properly. I think the example is sufficient.
  3. There is no support for reductions that require knowledge of the dimension values. This can be done in a later PR if the need arises and can be implemented as another AbstractApplyOperation.
  4. Error handling happens at materialization instead of at the construction of the SplitApplyVar. This can be changed later without it being a breaking change.
  5. The element of the new dimension is always the first element of each group. I don't think there need to be support for anything else, since I can't think of another use case for this.
  6. The attributes and dimension attributes are not updated. I wasn't too sure about this.

In the future, for the split operations, there would be support for arbitrary splits that the user can supply. Additionally, the apply operations can also include transformations (which keep the size of the array the same) and filter (requires adding a new dimension). An example of a transformation is doing a z-score transformation each group (e.g. seasons) and an example of a filter is getting the top k elements of each group. Both use cases are not needed right now, so they are not included in this PR.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.47%. Comparing base (8c9bfbd) to head (03fc2ca).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
+ Coverage   99.46%   99.47%   +0.01%     
==========================================
  Files          18       19       +1     
  Lines        2048     2096      +48     
==========================================
+ Hits         2037     2085      +48     
  Misses         11       11              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ph-kev ph-kev force-pushed the kp/split-apply-merge branch from 9cc61e2 to 0691e4c Compare May 15, 2026 16:25
@ph-kev ph-kev marked this pull request as ready for review May 15, 2026 16:25
Copy link
Copy Markdown
Member

@imreddyTeja imreddyTeja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I like this design pattern, but I don't really understand what the combine part does.

Comment thread docs/src/split_apply_combine.md Outdated
Comment thread docs/src/split_apply_combine.md Outdated
Comment thread src/split_apply_combine.jl
@ph-kev ph-kev force-pushed the kp/split-apply-merge branch 2 times, most recently from 2e5d3a8 to 4124042 Compare May 21, 2026 16:59
Copy link
Copy Markdown
Member

@nefrathenrici nefrathenrici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just nits

Comment thread test/test_split_apply_combine.jl
Comment thread NEWS.md Outdated
Comment thread src/split_apply_combine.jl Outdated
@ph-kev ph-kev force-pushed the kp/split-apply-merge branch from 4124042 to d03b9d2 Compare May 21, 2026 17:08
@ph-kev ph-kev force-pushed the kp/split-apply-merge branch from d03b9d2 to 03fc2ca Compare May 21, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better support for group by and reduce by

3 participants