Fix cudf.pandas datetimelike plotting failures (canonical freq offsets + Period/offset pickling) - #23054
Conversation
github-actions
Bot
added
Python
galipremsagar
added
bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesDatetimeIndex frequency internals
Pandas accelerator pickling support
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf/cudf/core/index.py`:
- Around line 3715-3723: The public freq property on Index currently returns a
pandas offset from the getter, but the setter path through _validate_freq only
accepts str or cudf.DateOffset, so assigning idx.freq back to itself can fail.
Update the freq setter/validation logic in Index so it normalizes pandas offset
objects returned by freq into the internal cudf.DateOffset representation before
storing, ensuring the getter and setter round-trip cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3c3dad96-e7d3-4b3b-aa92-fac154c626f1
📒 Files selected for processing (5)
python/cudf/cudf/core/dataframe.pypython/cudf/cudf/core/index.pypython/cudf/cudf/core/series.pypython/cudf/cudf/pandas/_wrappers/pandas.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
|
/okay to test c9cd350 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf/cudf/core/index.py`:
- Around line 3651-3653: The stepped-slice fast path in DatetimeIndex handling
should not multiply by _freq when _freq is None, since that can raise TypeError
for non-unit slice steps. Update the slice handling in the relevant index logic
around the slc.step fastpath to return None when _freq is missing, and only
compute slc.step * self._freq when a frequency is available so behavior matches
pandas.
- Around line 3428-3440: The freq serialization in Index.deserialize and the
matching header construction for generic pd.DateOffset currently drops
DateOffset.n and normalize because only kwds are stored, so the offset is
reconstructed with defaults. Update the freq payload handling in
cudf/core/index.py to include n and normalize alongside kwds when building
header["freq"], and in deserialize pass those values back into
pd.DateOffset(...) using the existing header["freq"] path so the original offset
is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4e813cd3-2ba0-4d24-83d7-d344bcf6ecda
📒 Files selected for processing (1)
python/cudf/cudf/core/index.py
| kwds[component] = c | ||
|
|
||
| return cudf.DateOffset(**kwds) | ||
| return cudf.DateOffset(**kwds)._maybe_as_fast_pandas_offset() |
There was a problem hiding this comment.
Are we at a stage where we could use pd.DateOffset(**kwds) directly?
There was a problem hiding this comment.
Not quite — a generic pd.DateOffset(days=1) never compares equal to the fast offsets pandas infers (pd.DateOffset(days=1) != pd.offsets.Day()) and its freqstr is not parseable, which would break freq comparisons and the freq round-trip through _validate_freq/serialization. to_offset(Timedelta(...)) is not a substitute either, since it yields <24 * Hours> for a daily delta instead of <Day>. So single-unit offsets still need converting to their fast pandas equivalents via _maybe_as_fast_pandas_offset; I added a comment at the call site in d791fe0 explaining this.
|
/okay to test fdb05e0 |
|
/merge |
3 participants
Description
Fixes failures in
tests/plotting/test_datetimelike.pyundercudf.pandas(14 of the 21 prior failures). Two root causes:1.
DatetimeIndex.freq/inferred_freqreturned cudf's internalDateOffsetThe public
freq/inferred_freqreturned acudf.DateOffset(e.g.<DateOffset: days=1>) rather than the canonical pandas offset (<Day>,<Minute>, ...). pandas plotting APIs reject this (PeriodDtype(freq)raisedTypeError: PeriodDtype argument should be string or BaseOffset, got DateOffset) and freq comparisons failed.These now return the canonical pandas offset via
_maybe_as_fast_pandas_offset(). The internal_freqkeeps the cudfDateOffsetfor serialization/arithmetic;inferred_freqis split into a private_inferred_freq(cudf offsets, used to populate_freq) and the publicinferred_freq(pandas offset). Internal callers inseries.py/dataframe.pyandDatetimeIndex.__init__/serializewere updated to use_freq/_inferred_freqaccordingly.Fixes
test_line_plot_datetime_frame[*](5) andtest_line_plot_inferred_freq[*](3).2. Pickling matplotlib figures containing
Period/ concrete offsetsMatplotlib date converters store
pandas.Periodx-data and concrete offsets (Day,Week, ...) asfreq. The module accelerator makes the corresponding module attributes resolve to proxies, so pickle's class-identity check failed (Can't pickle <class 'pandas.Period'>: it's not the same object as pandas.Period).Registered
copyregreducers forPeriod(proxy + real) and every concreteBaseOffsetsubclass, mirroring the existingTimestamp/DateOffsethandling. Fixestest_pickle_fig[*](6).The now-passing entries are removed from the
cudf.pandasxfail list.Not addressed (inherent)
The remaining
test_*_weekly_resampling/test_from_resampling_area_line_mixed*failures need a weekly weekday anchor (Week(weekday=4)), which cudf cannot infer (inferred_freqraises "Can't infer anchored week"); these remain xfailed.test_add_matplotlib_datetime64is a pandasxfail(GH9053) that xpasses under cudf.pandas due to matplotlib behavior, and stays skipped.Checklist