Skip to content

TEST: Remove deterministic temporal and rolling skips - #24015

Open
vyasr wants to merge 5 commits into
NVIDIA:mainfrom
vyasr:codex/remove-temporal-rolling-skips
Open

TEST: Remove deterministic temporal and rolling skips#24015
vyasr wants to merge 5 commits into
NVIDIA:mainfrom
vyasr:codex/remove-temporal-rolling-skips

Conversation

@vyasr

@vyasr vyasr commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Description

Replace temporal, reduction, and rolling parameter combinations that deterministically skipped before an assertion with their existing supported subsets. Every combination that previously ran, including its expected failures and warning checks, still runs. The rolling variance/std inputs are additionally module-scoped and read-only, so option variants reuse identical data rather than reconstructing it.

Checklist

  • I am familiar with the CONTRIBUTING.md guidelines.
  • New or existing tests cover these test-only changes.
  • Documentation updates are not needed.

@vyasr
vyasr requested a review from a team as a code owner September 6, 2026 23:48
@vyasr
vyasr requested a review from mroeschke September 6, 2026 23:48
@vyasr vyasr added tests Unit testing for project Python Affects Python cuDF API. Performance Performance related issue labels Sep 6, 2026
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f5103d33-6d76-46d7-81f2-156f89fa0b74

📥 Commits

Reviewing files that changed from the base of the PR and between a8ad204 and 8c1e3f3.

📒 Files selected for processing (4)
  • python/cudf/cudf/tests/indexes/timedeltaindex/test_binops.py
  • python/cudf/cudf/tests/series/methods/test_reductions.py
  • python/cudf/cudf/tests/series/test_binops.py
  • python/cudf/cudf/tests/window/test_rolling.py

Included review availability: Your plan provides up to 12 included reviews per hour; 5 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Tests
    • Expanded and clarified coverage for supported timedelta arithmetic and datetime/timedelta reductions.
    • Improved validation of rolling-window operations, including statistical reductions and grouped rolling calculations.
    • Reduced ambiguous test behavior by explicitly covering supported operation combinations and valid window configurations.
    • Reused consistent datasets across rolling statistical tests to improve coverage reliability and consistency.

Walkthrough

The test suite now parameterizes supported timedelta, datetime, and rolling operations explicitly. Runtime skips for unsupported operations and invalid rolling combinations were removed. Rolling variance and standard deviation tests reuse module-scoped fixtures.

Changes

Supported operation test coverage

Layer / File(s) Summary
Arithmetic and reduction coverage
python/cudf/cudf/tests/indexes/timedeltaindex/test_binops.py, python/cudf/cudf/tests/series/test_binops.py, python/cudf/cudf/tests/series/methods/test_reductions.py
Timedelta arithmetic and datetime/timedelta reduction tests now use explicit supported-operation lists instead of runtime skip branches.
Rolling reduction fixtures
python/cudf/cudf/tests/window/test_rolling.py
Rolling tests define supported reductions and reuse module-scoped window-size and generated-data fixtures for large variance and standard deviation cases.
Valid rolling parameter combinations
python/cudf/cudf/tests/window/test_rolling.py
Groupby rolling UDF tests generate only combinations where min_periods is less than or equal to window_size.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 8c1e3

This updates test collection to run supported temporal and rolling cases directly while reusing deterministic rolling inputs. No current merge-blocking risk remains.

Suggested reviewers: galipremsagar, matt711

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: removing deterministic skips from temporal and rolling tests.
Description check ✅ Passed The description accurately explains the test parameter filtering, preservation of runnable cases, rolling fixture reuse, and validation performed.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@vyasr vyasr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 7, 2026
@vyasr

vyasr commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test

@vyasr

vyasr commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test

@bdice bdice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving but not merging, see comment.

pytest.skip(f"{reduction_methods} not implemented")
return reduction_methods
@pytest.fixture(params=["min", "max", "sum", "std", "var"])
def supported_rolling_reductions(request):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The negative filter with explicit skips made it easier to see gaps in functionality. Not a blocker, just raising that for awareness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Performance Performance related issue Python Affects Python cuDF API. tests Unit testing for project

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants