Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion astropy/io/ascii/qdp.py

Choose a reason for hiding this comment

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

[major] The _command_re assignment appears syntactically incomplete. The inline flag group (?i:...) is missing its closing parenthesis, and the string literal lacks a closing quote. This will cause a syntax error and prevent parsing. Please fix to something like:

  • Minimal fix: r"(?i:READ [TS]ERR(\s+[0-9]+)+)"
  • More robust (anchor and allow trailing spaces): r"(?i)^READ [TS]ERR(?:\s+[0-9]+)+\s*$"

The inline flag is appropriately scoped to the command portion, so this change should only affect command matching and leave decimal/data/comment/new-table detection unchanged.

Choose a reason for hiding this comment

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

[minor] Consider anchoring the command regex and permitting optional leading/trailing whitespace to avoid false positives/negatives, e.g. r"(?i)^\s*READ [TS]ERR(?:\s+[0-9]+)+\s*$". This prevents a data line that happens to contain the substring from matching, and tolerates trailing spaces which are common in QDP files.

Choose a reason for hiding this comment

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

[nit] The note below the _command_re repeats the case-insensitivity statement already conveyed in the preceding comment. Consider condensing/removing the redundant line to keep comments crisp.

Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ def _line_type(line, delimiter=None):
ValueError: Unrecognized QDP line...
"""
_decimal_re = r"[+-]?(\d+(\.\d*)?|\.\d+)([eE][+-]?\d+)?"
_command_re = r"READ [TS]ERR(\s+[0-9]+)+"
# Recognize READ SERR/TERR directives case-insensitively.
# Allow tabs/multiple spaces between tokens; anchoring handled by _type_re.
_command_re = r"(?i:READ\s+[TS]ERR(?:\s+[0-9]+)+)"
# Note: Command directives (READ SERR/READ TERR) are recognized case-insensitively.

sep = delimiter
if delimiter is None:
Expand Down
122 changes: 122 additions & 0 deletions astropy/io/ascii/tests/test_qdp_case_insensitive.py

Choose a reason for hiding this comment

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

[major] The test file includes non-Python content at the end (lines like PYTEST and shell commands: git add -A, gh pr create, etc.). These will cause syntax errors and must be removed. The file should only contain test code.

Choose a reason for hiding this comment

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

[minor] Nice coverage of uppercase/lowercase/mixed cases and with/without explicit names. To strengthen robustness, consider adding tests for:

  • Tabs or multiple spaces between tokens (e.g., READ\tSERR\t3 or READ TERR 1)
  • Trailing whitespace on directive lines (READ SERR 3 )
  • A comment line following directives to ensure comment detection remains unaffected

These help confirm delimiter handling and that case-insensitive matching doesn’t interfere with other line-type detection.

Choose a reason for hiding this comment

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

[nit] Minor style: the two blank lines after the imports contain trailing spaces. Please strip trailing whitespace to satisfy linters.

Choose a reason for hiding this comment

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

[major] Several top-level statements are indented by one space (e.g., def _make_qdp_text(...), the @pytest.mark.parametrize(...) decorator, and the subsequent def test_qdp_command_case_insensitive(...)). Top-level defs/decorators must start at column 0; this will raise an IndentationError and break the test file. Please remove the leading spaces so these lines are flush-left.

Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import numpy as np
import pytest
from io import StringIO
from astropy.table import Table

def _make_qdp_text(cmd1, cmd2):
"""
Construct a minimal QDP text with two directives lines followed by
data lines. We flag column 1 with TERR (two-sided errors) and
column 3 with SERR (symmetric errors). The data lines then include
the values in appropriate sequence:
- For TERR on column 1: value, perr, nerr
- For SERR on column 3: value, err
The middle base column (column 2) has no errors.
"""
return (
f"{cmd1}\n"
f"{cmd2}\n"
"1 0.1 0.2 10 100 5\n"
"2 0.11 0.22 20 200 6\n"
)
@pytest.mark.parametrize(
"cmds",
[
("READ TERR 1", "READ SERR 3"), # uppercase
("read terr 1", "read serr 3"), # lowercase
("ReAd TeRr 1", "rEaD sErR 3"), # mixed case
],
)
def test_qdp_command_case_insensitive(cmds):
text = _make_qdp_text(*cmds)
# Explicit names: 'a', 'b', 'c'
t = Table.read(StringIO(text), format="ascii.qdp", names=["a", "b", "c"])

# Column presence
assert "a" in t.colnames
assert "b" in t.colnames
assert "c" in t.colnames
assert "a_perr" in t.colnames
assert "a_nerr" in t.colnames
assert "c_err" in t.colnames

# Values
assert np.allclose(t["a"], [1.0, 2.0])
assert np.allclose(t["a_perr"], [0.1, 0.11])
assert np.allclose(t["a_nerr"], [0.2, 0.22])
assert np.allclose(t["b"], [10.0, 20.0])
assert np.allclose(t["c"], [100.0, 200.0])
assert np.allclose(t["c_err"], [5.0, 6.0])


def test_qdp_command_case_insensitive_no_names():
# Lowercase directives to exercise case-insensitive detection
text = _make_qdp_text("read terr 1", "read serr 3")
t = Table.read(StringIO(text), format="ascii.qdp")

# Default behavior: base column names are auto-generated (e.g., col1..)
# Error columns should be created accordingly.
assert "col1" in t.colnames
assert "col2" in t.colnames
assert "col3" in t.colnames
assert "col1_perr" in t.colnames
assert "col1_nerr" in t.colnames
assert "col3_err" in t.colnames

assert np.allclose(t["col1"], [1.0, 2.0])
assert np.allclose(t["col1_perr"], [0.1, 0.11])
assert np.allclose(t["col1_nerr"], [0.2, 0.22])
assert np.allclose(t["col2"], [10.0, 20.0])
assert np.allclose(t["col3"], [100.0, 200.0])
assert np.allclose(t["col3_err"], [5.0, 6.0])


def test_qdp_command_uppercase_unchanged():
# Uppercase as per existing behavior; ensure unchanged
text = _make_qdp_text("READ TERR 1", "READ SERR 3")
t = Table.read(StringIO(text), format="ascii.qdp", names=["a", "b", "c"])

assert "a_perr" in t.colnames
assert "a_nerr" in t.colnames
assert "c_err" in t.colnames
assert np.allclose(t["a_perr"], [0.1, 0.11])
assert np.allclose(t["a_nerr"], [0.2, 0.22])
assert np.allclose(t["c_err"], [5.0, 6.0])


def test_qdp_directives_with_tabs_and_spaces():
# Mix tabs and multiple spaces between tokens in directives
cmd1 = "READ\tTERR\t1\t"
cmd2 = "READ SERR 3 "
text = _make_qdp_text(cmd1, cmd2)
t = Table.read(StringIO(text), format="ascii.qdp", names=["a", "b", "c"])

assert "a_perr" in t.colnames and "a_nerr" in t.colnames
assert "c_err" in t.colnames
assert np.allclose(t["a"], [1.0, 2.0])
assert np.allclose(t["a_perr"], [0.1, 0.11])
assert np.allclose(t["a_nerr"], [0.2, 0.22])
assert np.allclose(t["b"], [10.0, 20.0])
assert np.allclose(t["c_err"], [5.0, 6.0])


def test_qdp_directives_with_trailing_whitespace_and_comment_line():
# Directives with trailing whitespace and a following comment line
cmd1 = "read terr 1 \t"
cmd2 = "READ SERR 3 "
text = (
f"{cmd1}\n"
f"{cmd2}\n"
"! post-directive comment\n"
"1 0.1 0.2 10 100 5\n"
"2 0.11 0.22 20 200 6\n"
)

t = Table.read(StringIO(text), format="ascii.qdp", names=["a", "b", "c"])

# Ensure comment line does not interfere with line-type detection
assert "a_perr" in t.colnames and "a_nerr" in t.colnames
assert "c_err" in t.colnames
assert np.allclose(t["a"], [1.0, 2.0])
assert np.allclose(t["b"], [10.0, 20.0])
assert np.allclose(t["c_err"], [5.0, 6.0])