Skip to content

MaskedColumn finalize not giving expected results #1471

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.

Sign up for GitHub

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

Jump to bottom
Merged
astrofrog merged 6 commits into from
Dec 3, 2013
Merged

MaskedColumn finalize not giving expected results #1471

astrofrog merged 6 commits into from
Dec 3, 2013

Conversation

@astrofrog
Copy link
Member

@astrofrog astrofrog commented Dec 1, 2013

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

taldcroft added a commit to taldcroft/astropy that referenced this pull request Oct 13, 2013
@taldcroft
Put in workaround for issue astropy#1471
ff6039e
@astrofrog
Copy link
Member

astrofrog commented Nov 3, 2013

@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
Copy link
Member

astrofrog commented Dec 1, 2013

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

@astrofrog
Copy link
Member

astrofrog commented Dec 1, 2013

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

@astrofrog
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.
@astrofrog
Copy link
Member

astrofrog commented Dec 1, 2013

43440307-1

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

@astrofrog
Copy link
Member

astrofrog commented Dec 1, 2013

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

mhvk
mhvk reviewed Dec 1, 2013
View reviewed changes
astropy/table/column.py
Copy link
Contributor

@mhvk mhvk Dec 1, 2013

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@astrofrog astrofrog Dec 1, 2013

Choose a reason for hiding this comment

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

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
Copy link
Member

astrofrog commented Dec 2, 2013

@taldcroft - should I remove the workaround in ff6039e?

@taldcroft
Copy link
Member Author

taldcroft commented Dec 2, 2013

@astrofrog - will check into this later today.

@taldcroft
Copy link
Member Author

taldcroft commented Dec 2, 2013

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

@embray
Copy link
Member

embray commented Dec 2, 2013

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

@taldcroft
Copy link
Member Author

taldcroft commented Dec 2, 2013

@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.

@astrofrog
Copy link
Member

astrofrog commented Dec 2, 2013

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?

@mhvk mhvk mentioned this pull request Dec 2, 2013
Closed
@taldcroft
Copy link
Member Author

taldcroft commented Dec 2, 2013

The general approach of overriding take seems OK to me.

@astrofrog
Copy link
Member

astrofrog commented Dec 2, 2013

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.

@astrofrog
Copy link
Member

astrofrog commented Dec 2, 2013

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

@astrofrog
Copy link
Member

astrofrog commented Dec 3, 2013

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

@astrofrog
Copy link
Member

astrofrog commented Dec 3, 2013

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

@taldcroft
Copy link
Member Author

taldcroft commented Dec 3, 2013

This looks good to merge.

astrofrog added a commit that referenced this pull request Dec 3, 2013
@astrofrog
Merge pull request #1471 from astrofrog/issue-1471
97ff2b9 
MaskedColumn finalize not giving expected results
@astrofrog astrofrog merged commit 97ff2b9 into astropy:master Dec 3, 2013
@taldcroft taldcroft mentioned this pull request Dec 6, 2013
Merged
astrofrog added a commit that referenced this pull request Feb 12, 2014
@astrofrog @embray
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
@astrofrog @embray
Merge pull request #1471 from astrofrog/issue-1471
b5876fb 
MaskedColumn finalize not giving expected results
Conflicts:

	astropy/table/column.py
@astrofrog astrofrog deleted the issue-1471 branch July 5, 2016 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Bug 🔥 Critical table

Projects

None yet

Milestone

v0.3.1

Development

Successfully merging this pull request may close these issues.

4 participants

@astrofrog @taldcroft @embray @mhvk