Skip to content

FITS Table string related fails with Astropy dev - #1228

Merged
cdeil merged 6 commits into
gammapy:masterfrom
cdeil:1228
Dec 4, 2017
Merged

FITS Table string related fails with Astropy dev#1228
cdeil merged 6 commits into
gammapy:masterfrom
cdeil:1228

Conversation

@cdeil

@cdeil cdeil commented Dec 2, 2017

Copy link
Copy Markdown
Contributor

In Gammapy we are testing against the Astropy dev version, and this started to fail:
https://travis-ci.org/gammapy/gammapy/jobs/310116908#L3261

I think the cause is this FITS Table related change by @astrofrog that was merged yesterday:
astropy/astropy#6821

From a quick look, I think all the fails in gammapy.catalog originate from here, where we extract a FITS table row data into a dict that's independent from the table object:

gammapy/gammapy/catalog/core.py

Line 252 in 48d7ae1

def _make_source_dict(table, idx):

Before the change by @astrofrog FITS string column data came back as str, now as bytes (I only tested on Python 3 so far), and then our code fails later when we compare str with bytes (and previously worked because we compared str with str).

Here's an isolated test case: old behaviour:

>>> import astropy
>>> astropy.__version__
'2.0.2'
>>> from astropy.table import Table
>>> t = Table.read('https://github.com/gammapy/gammapy-extra/raw/master/datasets/catalogs/fermi/gll_psch_v08.fit.gz')
Downloading https://github.com/gammapy/gammapy-extra/raw/master/datasets/catalogs/fermi/gll_psch_v08.fit.gz
|==============================================================================================================| 205k/205k (100.00%)         0s
WARNING: hdu= was not specified but multiple tables are present, reading in first available table (hdu=1) [astropy.io.fits.connect]
WARNING: UnitsWarning: 'photon/cm**2/s' contains multiple slashes, which is discouraged by the FITS standard [astropy.units.format.generic]
WARNING: UnitsWarning: 'erg/cm**2/s' contains multiple slashes, which is discouraged by the FITS standard [astropy.units.format.generic]
>>> t['Source_Name'].data.dtype
dtype('<U18')
>>> t['Source_Name'].data[0]
'2FHL J0008.1+4709'
>>> type(t['Source_Name'].data[0])
<class 'numpy.str_'>

New behaviour:

>>> import astropy
>>> astropy.__version__
'3.0.dev20763'
>>> from astropy.table import Table
>>> t = Table.read('https://github.com/gammapy/gammapy-extra/raw/master/datasets/catalogs/fermi/gll_psch_v08.fit.gz')
Downloading https://github.com/gammapy/gammapy-extra/raw/master/datasets/catalogs/fermi/gll_psch_v08.fit.gz
|==============================================================================================================| 205k/205k (100.00%)         0s
WARNING: hdu= was not specified but multiple tables are present, reading in first available table (hdu=1) [astropy.io.fits.connect]
WARNING: UnitsWarning: 'photon/cm**2/s' contains multiple slashes, which is discouraged by the FITS standard [astropy.units.format.generic]
WARNING: UnitsWarning: 'erg/cm**2/s' contains multiple slashes, which is discouraged by the FITS standard [astropy.units.format.generic]
>>> t['Source_Name'].data.dtype
dtype('S18')
>>> t['Source_Name'].data[0]
b'2FHL J0008.1+4709'
>>> type(t['Source_Name'].data[0])
<class 'numpy.bytes_'>

In both cases, this gives a str:

>>> type(t['Source_Name'][0])
<class 'numpy.str_'>

Actually given https://github.com/astropy/astropy/pull/6821/files#diff-db23dcd814354c954091a9b90dbfd92aR253 this makes sense:

When reading in FITS tables with Table.read, string columns are now represented using Numpy byte (dtype S) arrays rather than Numpy unicode arrays (dtype U). The Column class then ensures the bytes are automatically converted to string as needed. [#6821]

@astrofrog So basically, this is an intentional change in Astropy, and we should update our code in Gammapy to no longer rely on column.data being a str for FITS string columns, no?

Concretely for the method _make_source_dict I linked to above I think that means we need an isinstance or try for string columns?

@cdeil cdeil added the bug label Dec 2, 2017
@cdeil cdeil added this to the 0.7 milestone Dec 2, 2017
@cdeil cdeil self-assigned this Dec 2, 2017
@cdeil cdeil mentioned this pull request Dec 2, 2017
Merged
@mhvk

mhvk commented Dec 2, 2017

Copy link
Copy Markdown

@cdeil - at some level you're bitten not by @astrofrog's changes but by the introduction of the unicode sandwich for Column that @taldcroft introduced in astropy/astropy#5700: by accessing the underlying data you now get bytes. To see whether we should revert it, it might be most useful to know the reason for accessing .data - is the idea is to get rid of the Column nature, perhaps? But if idx is guaranteed to be a row index, that should happen by indexing already (if not, it is a bug), and something like the following would suffice):

val = col[idx]
if col.unit and not isinstance(val, Quantity):
    val = Quantity(val, unit=col.unit)

@cdeil

cdeil commented Dec 2, 2017

Copy link
Copy Markdown
Contributor Author

@mhvk - Thanks for the info! I'll read over astropy/astropy#5700 and educate myself tomorrow.

What I want is a function that converts a Table row to an OrderedDict, with the extra feature that for columns with a unit, the value in the dict is a scalar quantity. So indeed, by not accessing .data, this works for our application (I didn't check on older Astropy yet):

    def table_row_to_dict(table, idx):
        data = OrderedDict()
        for name, col in table.columns.items():
            val = col[idx]
            if col.unit:
                val = val * col.unit
            data[name] = val
        return data

If there is a better (i.e. mostly faster) way to extract row data from a Table, please let me know.

@mhvk

mhvk commented Dec 2, 2017 •
edited
Loading

Copy link
Copy Markdown

@cdeil - don't use * col.unit - now if the table ever contains a mixin quantity column, you'll get the wrong unit!
This is why my example was:

if col.unit and not isinstance(val, Quantity):
    val = Quantity(val, unit=col.unit)

You could remove the isinstance [EDIT] which would make it faster for regular columns and with at best a little bit of speed loss for Quantity mixins.

@cdeil

cdeil commented Dec 2, 2017

Copy link
Copy Markdown
Contributor Author

@mhvk - I added a table_row_to_dict utility function for Gammapy in b72ceb7 following your advice how to implement it. Seems to work well, at least for my test case at hand. If you have time, could you have a quick look to make sure the implementation is OK now?

@mhvk

mhvk commented Dec 2, 2017

Copy link
Copy Markdown

@cdeil - that looks all OK (though one travis run failed; not sure whether that's related)

@cdeil

cdeil commented Dec 4, 2017

Copy link
Copy Markdown
Contributor Author

I think now all is good: table row -> dict or also the other way around row data as list of dict -> table always go through a helper function in gammapy.utils.table. I've also changed QTable -> Table in a few more places, i.e. #980 is almost done.

@cdeil
cdeil merged commit e266cf3 into gammapy:master Dec 4, 2017
alexcervino pushed a commit to alexcervino/gammapy that referenced this pull request Jul 4, 2026
@cdeil
Merge pull request gammapy#1228 from cdeil/1228
c1a6f17 
FITS Table string related fails with Astropy dev
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@cdeil cdeil

Labels

bug

Projects

None yet

Milestone

0.7

Development

Successfully merging this pull request may close these issues.

2 participants

@cdeil @mhvk