Skip to content

Comments

TimeSeries: clarify required columns error messages (swev-id: astropy__astropy-13033)#75

Open
casey-brooks wants to merge 1 commit intoastropy__astropy-13033from
noa/issue-65
Open

TimeSeries: clarify required columns error messages (swev-id: astropy__astropy-13033)#75
casey-brooks wants to merge 1 commit intoastropy__astropy-13033from
noa/issue-65

Conversation

@casey-brooks
Copy link

Summary

  • refine BaseTimeSeries._check_required_columns to emit list-style errors when multi-column requirements are violated
  • add regression coverage using TimeSeries to assert the clearer message for missing secondary required columns
  • ensure single-column validations retain their previous messaging

Reproduction Steps

  1. Create a TimeSeries and set ts._required_columns = ['time', 'flux']
  2. Insert a flux column at index 1, then remove it so the table starts ['time', 'a']
  3. Trigger the automatic check via ts.remove_column('flux')

Observed Failure (before this change)

ValueError: TimeSeries object is invalid - expected 'time' as the first column but found 'time'

New Behavior

  • Multi-column requirements now raise
    • ... required ['time', 'flux'] as the first columns but time series has no columns, or
    • ... required ['time', 'flux'] as the first columns but found ['time', 'a']
  • Single-column error messages are unchanged

Fixes #65

Testing

  • LD_LIBRARY_PATH=$HOME/.nix-profile/lib .venv/bin/pytest tmp_test_required_columns.py

@casey-brooks
Copy link
Author

Local verification:\n- LD_LIBRARY_PATH=/root/.nix-profile/lib .venv/bin/pytest tmp_test_required_columns.py (tests: 1 passed, 0 failed, 0 skipped)\n- LD_LIBRARY_PATH=/root/.nix-profile/lib .venv/bin/flake8 astropy/timeseries/core.py astropy/timeseries/tests/test_sampled.py (lint: no issues)

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Verified that BaseTimeSeries._check_required_columns preserves the single-column error message while multi-column prefixes report required/found lists. The new sampled time series test asserts the exact string when removing a required non-time column, and existing coverage for single-column behavior remains intact. Style touch-ups stay within project conventions.

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