Skip to content

Fix #606: numerical correctness assertions for Combine.stack/take tests + optimize Combine.stack()#607

Closed
Copilot wants to merge 4 commits intomainfrom
copilot/fix-issue-606-implementation
Closed

Fix #606: numerical correctness assertions for Combine.stack/take tests + optimize Combine.stack()#607
Copilot wants to merge 4 commits intomainfrom
copilot/fix-issue-606-implementation

Conversation

Copy link
Copy Markdown

Copilot AI commented Mar 25, 2026

Follow-up from #579 review. Two test gaps and one efficiency issue in Combine.

Test coverage gaps addressed:

  • test_combine_stack — now splits the grouped array into two real partitions, fits transforms on each independently, stacks them, and asserts np.testing.assert_allclose(stacked.update(full_ga), concat([u1, u2])) to verify numerical correctness rather than just structure.

  • test_nested_combine_take — now builds a subset_ga for the selected indices, fits a fresh transform on it, and asserts the take()-derived transform produces bit-identical update() output to the freshly-fitted one.

Efficiency fix:

Combine.stack() previously deepcopy(transforms[0]) and then immediately overwrote out.tfm1 / out.tfm2 — wasting the deep copy of those attributes. Fixed by creating a shallow stub with tfm1/tfm2 nullified before deepcopy, leaving the input object unmutated:

stub = copy.copy(first)
stub.tfm1 = stub.tfm2 = None
out = copy.deepcopy(stub)
out.tfm1 = first.tfm1.stack([tfm.tfm1 for tfm in transforms])
out.tfm2 = first.tfm2.stack([tfm.tfm2 for tfm in transforms])

Checklist:

  • This PR has a meaningful title and a clear description.
  • The tests pass.
  • All linting tasks pass.
  • The notebooks are clean.
Original prompt

Create a pull request that addresses all items described in GitHub issue #606 in the Nixtla/mlforecast repository:

  • Review the issue description and any linked comments, code snippets, stack traces, or reproduction steps.
  • Identify the root cause(s) in the codebase.
  • Implement a robust fix and any necessary refactors.
  • Add/extend automated tests to cover the reported behavior and prevent regressions.
  • Update documentation/examples if the fix changes user-facing behavior.

Issue reference (must be used as the source of requirements): #606

Acceptance criteria:

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Open a pr that tackles the things described in here

Assistant: I’ve queued up the pull request creation request for Nixtla/mlforecast based on issue #606, and I’m waiting for you to accept/approve it in the dialog so I can continue.

This pull request was created from Copilot chat.


💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 25, 2026

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ nasaul
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

…e Combine.stack()

Co-authored-by: nasaul <17485259+nasaul@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Nixtla/mlforecast/sessions/ceddc4d3-bc72-42d3-b400-94007b2b4cd0
Copilot AI changed the title [WIP] Fix issues described in GitHub issue #606 Fix #606: numerical correctness assertions for Combine.stack/take tests + optimize Combine.stack() Mar 25, 2026
Copilot AI requested a review from nasaul March 25, 2026 01:48
Copy link
Copy Markdown
Contributor

@nasaul nasaul left a comment

Choose a reason for hiding this comment

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

LGTM

@nasaul nasaul marked this pull request as ready for review April 2, 2026 17:01
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 2, 2026

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing copilot/fix-issue-606-implementation (9adbb69) with main (cb8b558)

Open in CodSpeed

@nasaul nasaul closed this Apr 15, 2026
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.

Follow-ups from Combine distributed fix (yherin/fix-distributed-combine-transform)

3 participants