Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed io.acii read IPAC tables for long column types on Windows #15992

Merged
merged 2 commits into from Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions astropy/io/ascii/ipac.py
Expand Up @@ -14,6 +14,8 @@
from textwrap import wrap
from warnings import warn

import numpy as np

from astropy.table.pprint import get_auto_format_func
from astropy.utils.exceptions import AstropyUserWarning

Expand Down Expand Up @@ -214,6 +216,10 @@ def get_cols(self, lines):
null = header_vals[3][i].strip()
fillval = "" if issubclass(col.type, core.StrType) else "0"
self.data.fill_values.append((null, fillval, col.name))
if "long".startswith(col.raw_type.lower()):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly confused by this line. Is it not doing more than it should ? Is, e.g. "Lo" a correct way to specify long ints ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure long can be abbreviated as Lo per IPAC spec, but current astropy implementation does allow all kind of abbreviation.

The above line mimics the existing ones in get_col_type() in

if col_type_key.startswith(col.raw_type.lower()):
:

The data type abbreviation has been explicitly tested in:

def test_ipac_abbrev():
lines = [
"| c1 | c2 | c3 | c4 | c5| c6 | c7 | c8 | c9|c10|c11|c12|",
"| r | rE | rea | real | D | do | dou | f | i | l | da| c |",

Copy link
Contributor

Choose a reason for hiding this comment

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

great, thank you very much for clarifying this !

Copy link
Contributor

Choose a reason for hiding this comment

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

Unresolving just so that others can see this too!

# ensure long columns are 64-bit int, to address:
# https://github.com/astropy/astropy/issues/15989
col.dtype = np.int64
start = col.end + 1
cols.append(col)

Expand Down
24 changes: 24 additions & 0 deletions astropy/io/ascii/tests/test_types.py
Expand Up @@ -53,6 +53,30 @@ def test_ipac_read_types():
assert_equal(col.type, expected_type)


def test_ipac_read_long_columns():
"""Test for https://github.com/astropy/astropy/issues/15989"""
test_data = """\
| oid|expid|cadence|
| long| l| int|
90000000000000001 123 1
"""
dat = ascii.read(test_data, format="ipac")

# assert oid, as a long column, is at the minimal int64
oid = dat["oid"]
assert oid[0] == 90000000000000001
assert oid.dtype.kind == "i"
assert oid.dtype.itemsize >= 8

# expid is declared as a long column,
# the type needs to be at least int64, even though all
# the values are within int32 range
expid = dat["expid"]
assert expid[0] == 123
assert expid.dtype.kind == "i"
assert expid.dtype.itemsize >= 8


def test_col_dtype_in_custom_class():
"""Test code in BaseOutputter._convert_vals to handle Column.dtype
attribute. See discussion in #11895."""
Expand Down
1 change: 1 addition & 0 deletions docs/changes/io.ascii/15992.bugfix.rst
@@ -0,0 +1 @@
Fixed reading IPAC tables for ``long`` column type on some platforms, e.g., Windows.