Skip to content

Ensure FITS_rec can be indexed with a tuple even if it returns a scalar. - #6955

Merged
saimn merged 1 commit into
astropy:masterfrom
mhvk:io-fits-fits_rec-scalar-indexing
Dec 10, 2017
Merged

Ensure FITS_rec can be indexed with a tuple even if it returns a scalar.#6955
saimn merged 1 commit into
astropy:masterfrom
mhvk:io-fits-fits_rec-scalar-indexing

Conversation

@mhvk

@mhvk mhvk commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

While investigating numpy-dev failures, I noticed one of them was caused by indexing a FITS_rec instance with a tuple of the form (number,), i.e., one that just returns a scalar. This causes a failure (see below) because the __getitem__ code assumes any tuple key will return a view on the original array (another example of why one should not LBYL but rather EAFP...). This fixes the problem, though scalar lookup will now become somewhat more expensive (probably, a real fix would be to deal with the reference issues that force one to go through the current hoops).

import numpy as np
from astropy.io.fits import FITS_rec
x = np.array([(1.0, 2), (3.0, 4)], dtype=[('x', float), ('y', int)]).view(FITS_rec)
x[1,]
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-1-aacf971fccce> in <module>()
      2 from astropy.io.fits import FITS_rec
      3 x = np.array([(1.0, 2), (3.0, 4)], dtype=[('x', float), ('y', int)]).view(FITS_rec)
----> 4 x[1,]

/usr/lib/python3/dist-packages/astropy/io/fits/fitsrec.py in __getitem__(self, key)
    490             subtype = type(self)
    491             out = self.view(np.recarray).__getitem__(key).view(subtype)
--> 492             out._coldefs = ColDefs(self._coldefs)
    493             arrays = []
    494             out._converted = {}

/usr/lib/python3/dist-packages/numpy/core/records.py in __setattr__(self, attr, val)
    265             else:
    266                 raise AttributeError("'record' object has no "
--> 267                         "attribute '%s'" % attr)
    268 
    269     def __getitem__(self, indx):

AttributeError: 'record' object has no attribute '_coldefs'

@mhvk mhvk added this to the v2.0.3 milestone Dec 8, 2017
@mhvk
mhvk requested a review from saimn December 8, 2017 22:36
@astropy-bot

astropy-bot Bot commented Dec 8, 2017 •
edited
Loading

Copy link
Copy Markdown

Hi there @mhvk 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@mhvk

mhvk commented Dec 8, 2017

Copy link
Copy Markdown
Contributor Author

ps. The reason this leads to failures on numpy-dev is that the new arrayprint routine indexes the array using tuples. So, we need to fix this to be able to use numpy-dev with fits tables, otherwise repr will break.

@bsipocz

bsipocz commented Dec 9, 2017

Copy link
Copy Markdown
Member

@mhvk - Is this critical for 2.0.3?

@mhvk

mhvk commented Dec 9, 2017

Copy link
Copy Markdown
Contributor Author

@bsipocz - yes if we want 2.0.3 to work with numpy 1.14. I'm fairly confident the fix is correct, but it would still be good to get some review (@saimn, @MSeifert04, @drdavella?).

@bsipocz

bsipocz commented Dec 9, 2017

Copy link
Copy Markdown
Member

OK, thanks for the confirmation.

saimn
saimn approved these changes Dec 10, 2017
View reviewed changes

@saimn saimn left a comment

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.

Looks good, thanks @mhvk

@saimn
saimn merged commit 21d6e36 into astropy:master Dec 10, 2017
@mhvk
mhvk deleted the io-fits-fits_rec-scalar-indexing branch December 10, 2017 18:14
bsipocz pushed a commit that referenced this pull request Dec 11, 2017
@saimn @bsipocz
Merge pull request #6955 from mhvk/io-fits-fits_rec-scalar-indexing
c62567a 
Ensure FITS_rec can be indexed with a tuple even if it returns a scalar.
@saimn saimn mentioned this pull request Dec 11, 2017
Merged
5 of 8 tasks
@bsipocz

bsipocz commented Dec 11, 2017

Copy link
Copy Markdown
Member

Changes here is not np1.9 compatible for the backport:
https://travis-ci.org/astropy/astropy/jobs/314820655

@mhvk

mhvk commented Dec 11, 2017

Copy link
Copy Markdown
Contributor Author

@bsipocz - one could try type(out) is not np.recarray instead of type(out) is np.record. Should I restore the branch to try that?

@bsipocz

bsipocz commented Dec 11, 2017

Copy link
Copy Markdown
Member

At this point I think the clearest would be to open a PR against the 2.0.x branch? Or if you are happy to have that change in master, against master and then I'll backport.

@mhvk

mhvk commented Dec 11, 2017

Copy link
Copy Markdown
Contributor Author

See #6966

@saimn saimn mentioned this pull request Jul 12, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saimn saimn saimn approved these changes

Assignees

No one assigned

Labels

Bug io.fits

Projects

None yet

Milestone

v2.0.3

Development

Successfully merging this pull request may close these issues.

3 participants

@mhvk @bsipocz @saimn