Skip to content

Fixes Issue #3505: Raise AttributeError on setting mask on unmasked column - #3522

Closed
quantshah wants to merge 1 commit into
astropy:masterfrom
quantshah:Issue-3505
Closed

quantshah wants to merge 1 commit into
astropy:masterfrom
quantshah:Issue-3505

Conversation

@quantshah

Copy link
Copy Markdown

Fix for issue #3505.

Comment thread astropy/table/column.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PEP8 (space after comma)

@taldcroft

Copy link
Copy Markdown
Member

@sahmed95 - looks like a good start, but you also need an explicit test of the exception being raised as expected. This should go in test_column.py.

@quantshah

Copy link
Copy Markdown
Author

Updated the commit: Added Test Case and followed PEP8

@taldcroft

Copy link
Copy Markdown
Member

@sahmed95 - there are widespread failures from this. You should work on this and make sure tests are passing locally before pushing changes up to github, for instance:

python setup.py test --package=table --args='--pdb'`

@quantshah

Copy link
Copy Markdown
Author

There seems to be a lot of places where mask is set on unmasked column by a functions in different modules.

___ TestFormatWithMaskedElements.test_column_format_func_with_special_masked ___
self = <astropy.table.tests.test_pprint.TestFormatWithMaskedElements instance at 0x1147d5a70>
    def test_column_format_func_with_special_masked(self):
        # run most of functions twice
        # 1) astropy.table.pprint._format_funcs gets populated
        # 2) astropy.table.pprint._format_funcs gets used

        t = Table([[1., 2., 3.], [3, 4, 5]], names=('a', 'b'), masked=True)
>       t['a'].mask = [True, False, True]
astropy/table/tests/test_pprint.py:445: 

The official numpy docs do not recommend the use of ".mask" to set mask value and instead use ma.mask :

From http://docs.scipy.org/doc/numpy/reference/maskedarray.generic.html :

A second possibility is to modify the mask directly, but this usage is discouraged.

Raising AttributeError while trying to set mask on an unmasked column might not be the best way then.
Is the issue more about warning the user that he/she is trying to set mask on a column which is not a MaskedColumn object ?

If that is the case then should a simple warning message be displayed saying that the user should probably use MaskedColumn if intending to set mask rather than raising AttributeError.
Else, a lot of changes have to be made in multiple places to accommodate this change.

@taldcroft

Copy link
Copy Markdown
Member

Incorrect implementation.

@taldcroft taldcroft closed this Feb 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants