MaskedColumn finalize not giving expected results by astrofrog · Pull Request #1471 · astropy/astropy

astrofrog · 2013-12-01T16:39:38Z

Slicing or doing other operations on a masked column doesn't propagate the other attributes:

>>> c = MaskedColumn(data=[1,2], name='a', description='a')
>>> print c[1:2].description
None

>>> c = Column(data=[1,2], name='a', description='a')
>>> print c[1:2].description
a

astrofrog · 2013-11-03T20:38:24Z

@taldcroft - you mentioned in ff6039e that there was a workaround for this issue, but it still doesn't seem to work. Any idea why?

astrofrog · 2013-12-01T16:10:00Z

Ah I see this was a workaround for another piece of functionality, but wasn't addressing the real issue here.

astrofrog · 2013-12-01T16:11:34Z

I'm going to look into this a bit now.

…lumn metadata. This required adding a workaround for a bug in Numpy which causes MaskedArray.__getitem__ to not call __array_finalize__. Fixes astropy#1471.

astrofrog · 2013-12-01T16:42:07Z

Seriously though, I guess I'll have to report this on the Numpy side and maybe submit a PR

astrofrog · 2013-12-01T16:42:26Z

@taldcroft - what do you think of the attached fix? Let's see if it works on Travis with all Numpy versions...

mhvk · 2013-12-01T17:45:09Z

To allow sub-classes to use this too, I would suggest:

self.__array_finalize__(obj) super(MaskedColumn, self)._update_from(obj)

Although I guess since this is for backporting, one could argue one might leave it as is, and I can just change it in #1848 if this one gets merged earlier.

Good point. This would need to get fixed in e.g. __array_finalize__ too, and maybe elsewhere, so I'd rather leave this to another pull request.

astrofrog · 2013-12-02T09:55:22Z

@taldcroft - should I remove the workaround in ff6039e?

taldcroft · 2013-12-02T14:13:40Z

@astrofrog - will check into this later today.

taldcroft · 2013-12-02T14:15:17Z

That numpy.ma image is hilarious. I almost want to make it into a stiicker for my laptop. 😄

embray · 2013-12-02T19:03:31Z

Yes, GitHub needs a "Like" button or similar.

taldcroft · 2013-12-02T21:46:48Z

@astrofrog - This looks good to me, thanks! Taking out the workaround in ff6039e seems good, but I would add a tests that metadata gets propagated with take() just to be 100% sure. I haven't looked into the code, but in some cases take is much faster than __getitem__, so it's going through a different code path at some level.

… test

astrofrog · 2013-12-02T22:19:15Z

ding ding ding ding ding we have a winner! @taldcroft has found a new bug in numpy.ma - MaskedArray.take also ignores __array_finalize__. In this case the workaround consists of simply overriding take since it also does not call update_from.

@taldcroft - what do you think? (assuming tests pass). Too hacky?

taldcroft · 2013-12-02T23:17:55Z

The general approach of overriding take seems OK to me.

astrofrog · 2013-12-02T23:41:04Z

Take does work with scalar values - but in the above case you'll only get something valid with 0 or 1 since the column has a length of 2. I'll add the tests anyway just to make sure it's all working fine.

… a function

astrofrog · 2013-12-02T23:46:47Z

@taldcroft - I've added tests for scalar values in take and have also added tests that np.take works (as a function)

astrofrog · 2013-12-03T10:49:01Z

@taldcroft - take does work with single scalar indices but only in Numpy 1.8+ - I've adjusted the tests to reflect this.

astrofrog · 2013-12-03T20:13:20Z

@taldcroft - should we merge this irrespective of the ongoing discussion about subclassing MaskedArray?

taldcroft · 2013-12-03T21:29:52Z

This looks good to merge.

MaskedColumn finalize not giving expected results

MaskedColumn finalize not giving expected results Conflicts: astropy/table/column.py

taldcroft added a commit to taldcroft/astropy that referenced this pull request Oct 13, 2013

Put in workaround for issue astropy#1471

ff6039e

Fix a problem that caused MaskedColumn.__getitem__ to not preserve co…

e6675a4

…lumn metadata. This required adding a workaround for a bug in Numpy which causes MaskedArray.__getitem__ to not call __array_finalize__. Fixes astropy#1471.

mhvk reviewed Dec 1, 2013
View reviewed changes

Also fix behavior for MaskedColumn with take and added regression…

e185729

… test

mhvk mentioned this pull request Dec 2, 2013

Construct ndarray/MaskedArray subclasses that can be re-used throughout astropy? #1857

Closed

Added docstring for MaskedColumn.take based on numpy.ndarray.take

1e2ee99

Added tests for scalar take and also added tests for using np.take as…

68b72c4

… a function

astrofrog added 2 commits December 3, 2013 00:48

Ensure arguments to take get passed through

41c4755

Take with single value only works with Numpy 1.8.0 and above.

3b9c8f9

astrofrog added a commit that referenced this pull request Dec 3, 2013

Merge pull request #1471 from astrofrog/issue-1471

97ff2b9

MaskedColumn finalize not giving expected results

astrofrog merged commit 97ff2b9 into astropy:master Dec 3, 2013

taldcroft mentioned this pull request Dec 6, 2013

Change the subclassing pattern for MaskedColumn #1872

Merged

astrofrog added a commit that referenced this pull request Feb 12, 2014

Merge pull request #1471 from astrofrog/issue-1471

59cc0b5

MaskedColumn finalize not giving expected results Conflicts: astropy/table/column.py

astrofrog added a commit that referenced this pull request Feb 12, 2014

Merge pull request #1471 from astrofrog/issue-1471

b5876fb

MaskedColumn finalize not giving expected results Conflicts: astropy/table/column.py

astrofrog deleted the issue-1471 branch July 5, 2016 15:49

Conversation

astrofrog commented Dec 1, 2013

Uh oh!

astrofrog commented Nov 3, 2013

Uh oh!

astrofrog commented Dec 1, 2013

Uh oh!

astrofrog commented Dec 1, 2013

Uh oh!

astrofrog commented Dec 1, 2013

Uh oh!

astrofrog commented Dec 1, 2013

Uh oh!

mhvk Dec 1, 2013

Choose a reason for hiding this comment

Uh oh!

astrofrog Dec 1, 2013

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Dec 2, 2013

Uh oh!

taldcroft commented Dec 2, 2013

Uh oh!

taldcroft commented Dec 2, 2013

Uh oh!

embray commented Dec 2, 2013

Uh oh!

taldcroft commented Dec 2, 2013

Uh oh!

astrofrog commented Dec 2, 2013

Uh oh!

taldcroft commented Dec 2, 2013

Uh oh!

astrofrog commented Dec 2, 2013

Uh oh!

astrofrog commented Dec 2, 2013

Uh oh!

astrofrog commented Dec 3, 2013

Uh oh!

astrofrog commented Dec 3, 2013

Uh oh!

taldcroft commented Dec 3, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants