Skip to content

fix: reject nulls in id and time columns in validate_format - #322

Merged
José Morales (jmoralez) merged 2 commits into
mainfrom
worktree-validate-nulls
Oct 26, 2026
Merged

José Morales (jmoralez) merged 2 commits into
mainfrom
worktree-validate-nulls

Conversation

@jmoralez

Copy link
Copy Markdown
Contributor

What

validate_format now raises a ValueError when the id or time column contains null values (None, NaN, NaT or NA in pandas; nulls in polars). The check runs even when target_col=None.

Why

Null ids and times aren't supported, but nothing rejected them, so they either failed far from the cause or produced wrong results. For example, polars process_df with ids ['a', 'a', None, 'b'] silently returned an indptr that gave the null series a row from 'a' and 'a' the null row, while pandas raised TypeError: '<=' not supported between instances of 'str' and 'NoneType'. Since process_df, fill_gaps, pipeline, rectify and plot_series all call validate_format, they now fail early with a clear message.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jmoralez
José Morales (jmoralez) marked this pull request as ready for review October 22, 2026 22:20
Copilot AI balanced review requested due to automatic review settings October 22, 2026 22:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused checks run before the target-column early return, with regression coverage for both dataframe backends and no unresolved findings.

No open issues 🎉

What changed in this PR

This PR adds early rejection of null identifiers and timestamps to shared dataframe validation, preventing downstream processing errors and incorrect results.

Changes:

  • Reject null id and time values in pandas and polars, including when target_col=None.
  • Add validation tests across supported column types and null representations.
  • Add a process_df regression test for null identifiers in both backends.
File Description
utilsforecast/​validation.py Adds backend-specific null detection and id/time validation.
tests/​test_validation.py Tests null rejection with and without target validation.
tests/​test_processing.py Verifies process_df rejects null identifiers early.

Review effort: Balanced

def _has_nulls(s: Series) -> bool:
if isinstance(s, pd.Series):
return s.hasnans
return s.null_count() > 0

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.

null_count() doesn't count NaN in polars, so a float id column with NaN passes here while the pandas branch (hasnans) rejects it:

validate_format(pl.DataFrame({'unique_id': [1.0, float('nan')], 'ds': [1, 2], 'y': [1., 2.]}))  # no error

Those ids then reach process_df/fill_gaps and get misgrouped. Something like s.null_count() > 0 or (s.dtype.is_float() and s.is_nan().any()) would match pandas. Adding pl.Series([1.0, float('nan')]) to test_validate_format_null_ids would cover it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NaNs get grouped together in polars

Comment thread utilsforecast/validation.py Resolved
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The checks are correctly placed, preserve optional-target behavior, and have focused regression coverage with no unresolved findings.

No open issues 🎉

Review effort: Balanced

@jmoralez
José Morales (jmoralez) merged commit 1587d34 into main Oct 26, 2026
22 checks passed
@jmoralez
José Morales (jmoralez) deleted the worktree-validate-nulls branch October 26, 2026 22:09
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.

3 participants