Fix problem comparing unicode to bytes masked column (#6838) by taldcroft · Pull Request #6899 · astropy/astropy

taldcroft · 2017-11-26T20:16:47Z

This PR addresses #6838 and #6839. The first appears to be an upstream problem in numpy MaskedArray (see #6838 comments). This PR improves testing and implements a narrow fix for the only case that previously failed. For #6839 there was an issue that the encoding dropped the masked array. Fixing this required the fix for #6838 so they are combined in one PR.

Fixes #6838
Fixed #6839

astropy-bot · 2017-11-26T20:16:48Z

Hi there @taldcroft 👋 - 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 · 2017-11-26T20:23:00Z

+            # case just swap self and other.
+            if (isinstance(self, MaskedColumn) and self.dtype.kind == 'U' and
+                    isinstance(other, MaskedColumn) and other.dtype.kind == 'S'):
+                self, other = other, self


This is OK only for == and !=, not for the other comparisons...

Good point. I actually had written a < compare into the tests but then deleted.

Actually, have you tried a simpler option, of just returning NotImplemented? python should then automatically call the reverse operator:

In [6]: class A: ...: def __lt__(self, other): ...: return NotImplemented ...: def __gt__(self, other): ...: return 'Yes' ...: In [7]: a = A() In [8]: a > a Out[8]: 'Yes' In [9]: a < a Out[9]: 'Yes'

I think this would not work for __eq__ and __ne__, though, since those do not have a different swapped version. So perhaps the clarity of this implementation still is preferable.

I implemented the suggestion of defining swapped_oper early, which does look cleaner (and saves some memory and 98 ns per compare). On this idea I think that code simplicity wins.

taldcroft · 2017-11-26T22:45:35Z

It turns out fixing #6839 required this fix in this PR so I combined them.

astrofrog

This looks great, thanks! I think @mhvk should probably have one last look

mhvk · 2017-11-27T15:20:13Z

        Make comparison methods which encode the ``other`` object to utf-8
        in the case of a bytestring dtype for Py3+.
        """
+        swap_op = {'__eq__': '__eq__',


Minor nitpick - since this is inside _make_compare and you know oper already here, I would defined

swapped_oper = {....}[oper]

and then do op = swapped_oper inside the if statement in _compare - that way the dict look-up doesn't have to happen every time a comparison is done (and possibly it ensures we don't get six versions of the same dict hanging around - not quite sure how python does this internally, but it seems likely it would create a new dict for every call to _make_compare which would then hang around since it is used in _compare).

Even more minor nitpick: you could remove __eq__ and __ne__ if you do {...}.get(oper, oper) - saves another two references to needless strings... But arguably the clarity provided by having all is more important.

mhvk · 2017-11-27T15:43:59Z

It looks like this isn't a bug on numpy >=1.13 (#6838 (comment)) - so, we'd need an if NUMPY_LT_1_13 statement.

I wonder if we shouldn't just make that the minimal version, since as that introduced __array_ufunc__... I'll send an e-mail to the mailing list with rationale.

taldcroft · 2017-11-27T19:47:52Z

If I put in this change to test for numpy < 1.13 and run with 1.13, then the current PR fails tests deep in np.ma.core._comparison. I'm inclined to just stick with the original version and be done with it. I could add a code comment with NUMPY_LT_1_13 in it to remind ourselves to look at this again when numpy 1.12 is no longer supported?

             # Special case to work around #6838.  Other combinations work OK,
             # see tests.test_column.test_unicode_sandwich_compare().  In this
-            # case just swap self and other.
-            if (isinstance(self, MaskedColumn) and self.dtype.kind == 'U' and
+            # case just swap self and other.  Fixed in numpy 1.13.
+            if (NUMPY_LT_1_13 and
+                    isinstance(self, MaskedColumn) and self.dtype.kind == 'U' and
                     isinstance(other, MaskedColumn) and other.dtype.kind == 'S'):
                 self, other = other, self

mhvk · 2017-11-27T21:09:00Z

@taldcroft - OK, sad to hear that the full tests didn't work on numpy 1.13. I agree that in that case we should keep it simple, and like the comment you added. So, I'll go ahead and merge!

p.s. If you have a minimal example with MaskedColumn that fails, could you post it here? Saves me trying to figure it out...

taldcroft · 2017-11-27T21:11:16Z

Crossed paths in the air. Using astropy master astropy=3.0.dev20678 and numpy 1.13.1. It does not give an exception but doesn't give the right answer either.

In [6]: MaskedColumn(['a', 'b']) == MaskedColumn([b'b', b'b'])
Out[6]: False

In [7]: MaskedColumn([b'a', b'b']) == MaskedColumn(['b', 'b'])
Out[7]: 
masked_array(data = [False  True],
             mask = False,
       fill_value = True)

Fix problem comparing unicode to bytes masked column (#6838)

bsipocz · 2017-12-09T17:42:58Z

Some of the tests here fail for python2 (on the bugfix branch). I would suspect that it's enough to just simply fix the tests, but let me know if I'm on the wrong track.

=================================== FAILURES ===================================

_______________ test_unicode_sandwich_compare[str-MaskedColumn] ________________

class1 = <class 'astropy.table.column.MaskedColumn'>, class2 = <type 'str'>

    @pytest.mark.parametrize('class1', [table.MaskedColumn, table.Column])

    @pytest.mark.parametrize('class2', [table.MaskedColumn, table.Column, str, list])

    def test_unicode_sandwich_compare(class1, class2):

        """Test that comparing a bytestring Column/MaskedColumn with various

        str (unicode) object types gives the expected result.  Tests #6838.

        """

        obj1 = class1([b'a', b'c'])

        if class2 is str:

            obj2 = 'a'

        elif class2 is list:

            obj2 = ['a', 'b']

        else:

            obj2 = class2(['a', 'b'])

    

        assert np.all((obj1 == obj2) == [True, False])

>       assert np.all((obj2 == obj1) == [True, False])

E       AssertionError

astropy/table/tests/test_column.py:793: AssertionError

__________________ test_unicode_sandwich_compare[str-Column] ___________________

class1 = <class 'astropy.table.column.Column'>, class2 = <type 'str'>

    @pytest.mark.parametrize('class1', [table.MaskedColumn, table.Column])

    @pytest.mark.parametrize('class2', [table.MaskedColumn, table.Column, str, list])

    def test_unicode_sandwich_compare(class1, class2):

        """Test that comparing a bytestring Column/MaskedColumn with various

        str (unicode) object types gives the expected result.  Tests #6838.

        """

        obj1 = class1([b'a', b'c'])

        if class2 is str:

            obj2 = 'a'

        elif class2 is list:

            obj2 = ['a', 'b']

        else:

            obj2 = class2(['a', 'b'])

    

        assert np.all((obj1 == obj2) == [True, False])

>       assert np.all((obj2 == obj1) == [True, False])

E       AssertionError

astropy/table/tests/test_column.py:793: AssertionError

=============================== warnings summary ===============================

bsipocz · 2017-12-09T19:34:19Z

I've added an ugly workaround hack for the tests for the bugfix branch in #6943. Let me know if you would rather not backport this back, or have a fix in mind.

Fix problem comparing unicode to bytes masked column

3d29c9d

taldcroft added Affects-release Bug table labels Nov 26, 2017

taldcroft added this to the v2.0.3 milestone Nov 26, 2017

taldcroft requested review from astrofrog and mhvk November 26, 2017 20:16

mhvk reviewed Nov 26, 2017

View reviewed changes

taldcroft added 2 commits November 26, 2017 16:34

Fix problem for all operators and test

2d0dcb1

Fix problem comparing masked columns in bytes and unicode

94865ea

astrofrog approved these changes Nov 27, 2017

View reviewed changes

astrofrog mentioned this pull request Nov 27, 2017

Make it possible to use Table.read on FITS files with no copying #6821

Merged

mhvk reviewed Nov 27, 2017

View reviewed changes

Improve code logic and leave breadcrumbs for future maintainers

d5a6756

mhvk merged commit 63fc64d into astropy:master Nov 27, 2017

taldcroft mentioned this pull request Nov 27, 2017

Bug in Table with unicode sandwich related to order of comparison #6838

Closed

taldcroft deleted the table-fix-6838 branch November 27, 2017 21:12

bsipocz pushed a commit that referenced this pull request Nov 28, 2017

Merge pull request #6899 from taldcroft/table-fix-6838

bcba2e8

Fix problem comparing unicode to bytes masked column (#6838)

bsipocz mentioned this pull request Dec 9, 2017

Fixing version dependent CI failures #6943

Merged

Conversation

taldcroft commented Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot Bot commented Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk Nov 26, 2017

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 26, 2017

Choose a reason for hiding this comment

Uh oh!

mhvk Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Nov 26, 2017

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

mhvk commented Nov 27, 2017

Uh oh!

taldcroft commented Nov 27, 2017

Uh oh!

mhvk commented Nov 27, 2017

Uh oh!

taldcroft commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Dec 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

taldcroft commented Nov 26, 2017 •

edited

Loading

astropy-bot Bot commented Nov 26, 2017 •

edited

Loading

taldcroft Nov 27, 2017 •

edited

Loading

taldcroft commented Nov 27, 2017 •

edited

Loading

bsipocz commented Dec 9, 2017 •

edited

Loading