Skip to content

BUG: Further fixes to record and recarray getitem/getattr #5921

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

Merged
merged 1 commit into from
Jun 2, 2015

Conversation

embray
Copy link
Contributor

@embray embray commented May 27, 2015

This is a followup to PR #5505, which didn't go quite far
enough. This fixes two issues in particular:

  1. The record class also needs an updated __getitem__ that works analogously to its __getattribute__ so that a nested record is returned as a record object and not a plain np.void. In other words the old behavior is:
>>> rec = np.rec.array([('abc ', (1,1), 1), ('abc', (2,3), 1)],
...       dtype=[('foo', 'S4'), ('bar', [('A', int), ('B', int)]),
('baz', int)])
>>> rec[0].bar
(1, 1)
>>> type(rec[0].bar)
<class 'numpy.record'>
>>> type(rec[0]['bar'])
<type 'numpy.void'>

demonstrated inconsistency between .bar and ['bar'] on the record object. The new behavior is:

>>> type(rec[0]['bar'])
<class 'numpy.record'>
  1. The second issue is more subtle. The fix to BUG: Fix recarray getattr and getindex return types #5505 used the obj.dtype.descr attribute to create a new dtype of type record. However, this does not recreate the correct type if the fields are not aligned. To demonstrate:
>>> dt = np.dtype({'C': ('S5', 0), 'D': ('S5', 6)})
>>> dt.fields
dict_proxy({'C': (dtype('S5'), 0), 'D': (dtype('S5'), 6)})
>>> dt.descr
[('C', '|S5'), ('', '|V1'), ('D', '|S5')]
>>> new_dt = np.dtype((np.record, dt.descr))
>>> new_dt
dtype((numpy.record, [('C', 'S5'), ('f1', 'V1'), ('D', 'S5')]))
>>> new_dt.fields
dict_proxy({'f1': (dtype('V1'), 5), 'C': (dtype('S5'), 0), 'D':
(dtype('S5'), 6)})

Using the fields dict to construct the new type reconstructs the correct type with the correct offsets:

>>> new_dt2 = np.dtype((np.record, dt.fields))
>>> new_dt2.fields
dict_proxy({'C': (dtype('S5'), 0), 'D': (dtype('S5'), 6)})

(Note: This is based on #5920 for convenience, but I could decouple the changes if that's preferable.)

embray added a commit to embray/astropy that referenced this pull request May 27, 2015
…commit message) numpy-dev. Some of the tests are also fixed by numpy/numpy#5921 it doesn't get merged before Numpy 1.10 is out a workaround will have to be developed.
@charris
Copy link
Member

charris commented May 27, 2015

@embray Could you fix up the commit messages and maybe squash some? Summaries like "Further fixes to record and recarray getitem/getattr" are a bit short on specificity.

@embray
Copy link
Contributor Author

embray commented May 27, 2015

@charris This PR really just includes 1 commit (c14a4c4) The rest are for #5920, on which this is based. If it's all the same to you I'll squash the commits on that one.

@charris
Copy link
Member

charris commented May 27, 2015

OK.

@embray embray force-pushed the record-getitem-fixes branch from c14a4c4 to f2af07d Compare May 27, 2015 22:15
@embray
Copy link
Contributor Author

embray commented May 27, 2015

Rebased on the squashed version of #5920.

@embray
Copy link
Contributor Author

embray commented May 28, 2015

Travis build failed at

numpy/core/src/multiarray/multiarraymodule_onefile.c:62:0: fatal error: error writing to /tmp/ccBBae11.s: No space left on device

    compilation terminated.

    The bug is not reproducible, so it is likely a hardware or OS problem.

so unrelated.

This is a followup to PR numpy#5505, which didn't go quite far
enough.  This fixes two issues in particular:

1) The record class also needs an updated `__getitem__` that works
analogously to its `__getattribute__` so that a nested record is
returned as a `record` object and not a plain `np.void`.  In other
words the old behavior is:

```python
>>> rec = np.rec.array([('abc ', (1,1), 1), ('abc', (2,3), 1)],
...       dtype=[('foo', 'S4'), ('bar', [('A', int), ('B', int)]),
('baz', int)])
>>> rec[0].bar
(1, 1)
>>> type(rec[0].bar)
<class 'numpy.record'>
>>> type(rec[0]['bar'])
<type 'numpy.void'>
```

demonstrated inconsistency between `.bar` and `['bar']` on the record
object.  The new behavior is:

```python
>>> type(rec[0]['bar'])
<class 'numpy.record'>
```

2) The second issue is more subtle.  The fix to numpy#5505 used the
`obj.dtype.descr` attribute to create a new dtype of type `record`.
However, this does not recreate the correct type if the fields are
not aligned.  To demonstrate:

```python
>>> dt = np.dtype({'C': ('S5', 0), 'D': ('S5', 6)})
>>> dt.fields
dict_proxy({'C': (dtype('S5'), 0), 'D': (dtype('S5'), 6)})
>>> dt.descr
[('C', '|S5'), ('', '|V1'), ('D', '|S5')]
>>> new_dt = np.dtype((np.record, dt.descr))
>>> new_dt
dtype((numpy.record, [('C', 'S5'), ('f1', 'V1'), ('D', 'S5')]))
>>> new_dt.fields
dict_proxy({'f1': (dtype('V1'), 5), 'C': (dtype('S5'), 0), 'D':
(dtype('S5'), 6)})
```

Using the `fields` dict to construct the new type reconstructs the
correct type with the correct offsets:

```python
>>> new_dt2 = np.dtype((np.record, dt.fields))
>>> new_dt2.fields
dict_proxy({'C': (dtype('S5'), 0), 'D': (dtype('S5'), 6)})
```

(Note: This is based on numpy#5920 for convenience, but I could decouple
the changes if that's preferable.)
@embray embray force-pushed the record-getitem-fixes branch from f2af07d to 776c3c3 Compare June 1, 2015 16:51
@embray
Copy link
Contributor Author

embray commented Jun 1, 2015

Rebased on master, which now includes #5920

charris added a commit that referenced this pull request Jun 2, 2015
BUG: Further fixes to record and recarray getitem/getattr
@charris charris merged commit f535f71 into numpy:master Jun 2, 2015
@charris
Copy link
Member

charris commented Jun 2, 2015

Thanks @embray.

@embray embray deleted the record-getitem-fixes branch June 2, 2015 21:45
dhomeier pushed a commit to dhomeier/astropy that referenced this pull request Aug 11, 2015
…commit message) numpy-dev. Some of the tests are also fixed by numpy/numpy#5921 it doesn't get merged before Numpy 1.10 is out a workaround will have to be developed.
embray added a commit to embray/PyFITS that referenced this pull request Jan 19, 2016
…commit message) numpy-dev. Some of the tests are also fixed by numpy/numpy#5921 it doesn't get merged before Numpy 1.10 is out a workaround will have to be developed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants