Skip to content

fix(#220): PandasAdapter auto time-column detection was unreachable dead code#238

Open
himax12 wants to merge 1 commit intosktime:mainfrom
himax12:fix/pandas-adapter-time-column-detection
Open

fix(#220): PandasAdapter auto time-column detection was unreachable dead code#238
himax12 wants to merge 1 commit intosktime:mainfrom
himax12:fix/pandas-adapter-time-column-detection

Conversation

@himax12
Copy link
Copy Markdown

@himax12 himax12 commented Apr 18, 2026

Summary

The condition isinstance(df.index, (pd.DatetimeIndex, pd.RangeIndex, pd.Index)) was ALWAYS True because pd.Index is the base class of ALL pandas indexes. This made the 'not' always False, making _detect_time_column() unreachable dead code.

The Fix

Removed pd.Index from the tuple:

  • Before: isinstance(df.index, (pd.DatetimeIndex, pd.RangeIndex, pd.Index)) # Always True!
  • After: isinstance(df.index, (pd.DatetimeIndex, pd.RangeIndex))

This allows auto-detection to run when index is not DatetimeIndex or RangeIndex.

Changes

  • src/sktime_mcp/data/adapters/pandas_adapter.py: Removed pd.Index from the isinstance check

Testing

  • DatetimeIndex index -> skip detection (correct)
  • RangeIndex index -> skip detection (correct)
  • Plain Index (strings/numbers) -> trigger auto-detection (now works!)

Copilot AI review requested due to automatic review settings April 18, 2026 07:21
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes unreachable auto time-column detection in PandasAdapter.load() by correcting an isinstance() check that previously always evaluated as true due to inclusion of pd.Index.

Changes:

  • Remove pd.Index from the df.index type check so _detect_time_column() can run for non-datetime/non-range indexes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +54 to 55
elif not isinstance(df.index, (pd.DatetimeIndex, pd.RangeIndex)):
# Try to detect time column
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

After removing pd.Index from this check, _detect_time_column() will now run when df.index is a pd.PeriodIndex (and other pd.Index subclasses like MultiIndex). Since validate() explicitly treats PeriodIndex as a supported time index, running auto-detection here can unexpectedly overwrite an existing valid time index via df.set_index(time_col). Consider adding pd.PeriodIndex to the tuple (and/or explicitly skipping auto-detection for other already-acceptable index types) so only truly "generic"/non-time-like indexes trigger detection.

Suggested change
elif not isinstance(df.index, (pd.DatetimeIndex, pd.RangeIndex)):
# Try to detect time column
elif not isinstance(df.index, (pd.DatetimeIndex, pd.RangeIndex, pd.PeriodIndex)):
# Try to detect time column only when there is no already-supported time index

Copilot uses AI. Check for mistakes.
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.

2 participants