Table > value should not work by astrofrog · Pull Request #828 · astropy/astropy

astrofrog · 2013-09-20T01:50:03Z

In Python 2, the following works:

In [1]: from astropy.table import Table

In [2]: t = Table([[1,2,3]])

In [3]: t > 0.2
Out[3]: True

but I think it shouldn't, and indeed doesn't in Python 3:

In [1]: from astropy.table import Table

In [2]: t = Table([[1,2,3]])

In [3]: t > 0.2
ERROR: TypeError: unorderable types: Table() > float() [IPython.core.interactiveshell]
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-26baf2ef9239> in <module>()
----> 1 t > 0.2

TypeError: unorderable types: Table() > float()

The Python 2 behavior led me to confusion when I did t[t > 0.2] by mistake, when I meant t[t['RA'] > 0.2].

taldcroft · 2013-02-27T03:16:48Z

That's a generic property of object in Python 2.

In [1]: class A(object):
   ...:     pass
   ...: 
In [2]: a = A()
In [3]: a > 2
Out[3]: True

Do you think we should overload all the comparisons to allow raising an exception?

astrofrog · 2013-02-27T07:52:45Z

I think it would be safest to disable comparisons, just because the whole t[t['col'] > 2] is such a useful syntax that it will be easy to make mistakes. I don't feel strongly about this, so I'll let you decide.

astrofrog · 2013-09-20T01:39:48Z

Interestingly this is not an issue in Python 3:

In [2]: class A(object):
   ...:     pass
   ...: 

In [3]: a = A()

In [4]: a > 2
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-6cf549916675> in <module>()
----> 1 a > 2

TypeError: unorderable types: A() > int()

Pull request will follow for Python 2

astrofrog · 2013-09-20T01:51:21Z

The attached code disables inequality comparisons for Python 2 (they already are in Python 3). Note that Numpy structured arrays also have this 'feature':

In [4]: np.array(list(zip([1,2,3],[2,3,4])), dtype=[('a', float), ('b', float)]) > 1
Out[4]: True

but I think it will just lead to confusion.

mdboom · 2013-09-20T12:55:11Z

@astrofrog: As you say this is already disabled on Python 3, is there any benefit to only defining these functions on Python 2?

Good point - how would I go about doing this?

astrofrog · 2013-09-28T11:36:58Z

@taldcroft @mdboom - does this look better?

taldcroft · 2013-09-28T11:43:26Z

Looks fine to me.

taldcroft · 2013-09-29T22:50:41Z

I changed my mind on __eq__ and __ne__. I think they should follow the behavior of numpy structured arrays, which is to give return the (in)equality of each row.

In [5]: t = Table.read([' a b c d',
                 ' 2 c 7.0 0',
                 ' 2 b 5.0 1',
                 ' 2 b 6.0 2',
                 ' 2 a 4.0 3',
                 ' 0 a 0.0 4',
                 ' 1 b 3.0 5',
                 ' 1 a 2.0 6',
                 ' 1 a 1.0 7',
                 ], format='ascii')
In [8]: t._data == t._data
Out[8]: array([ True,  True,  True,  True,  True,  True,  True,  True], dtype=bool)

In [9]: t._data != t._data
Out[9]: array([False, False, False, False, False, False, False, False], dtype=bool)

So I think the Table.__eq__ should basically return self._data == other. Will that work?

taldcroft · 2013-09-29T23:06:25Z

    def __eq__(self, other):
        if isinstance(other, Table):
            other = other._data
        return self._data == other

    def __ne__(self, other):
        if isinstance(other, Table):
            other = other._data
        return self._data != other

…not allowed on tables

… Also disable equality comparison.

mdboom · 2013-09-30T15:05:47Z

Looks good to me, and I agree with @taldcroft about __eq__ and __ne__.

astrofrog · 2013-09-30T15:10:33Z

Ok, great! The above commits implement what @taldcroft was suggesting, so I think this is good to merge. However, I'll let @taldcroft check first.

taldcroft · 2013-09-30T16:01:58Z

Can you add tests for comparing to a numpy structured array on the left and right sides? I assume this will work, but am not 100% sure.

t2._data == t t2 == t._data

astrofrog · 2013-09-30T16:29:08Z

@taldcroft - you're not going to believe this, it looks like there's a bug in numpy.ma ;)

In [75]: x = np.ma.MaskedArray([(2,),(2,)], dtype = [('a', '<i8')])

In [76]: y = np.array([(2,), (2,)], dtype=[('a', '<i8')])

In [77]: x == y
...
IndexError: arrays used as indices must be of integer (or boolean) type

Working on a workaround...

taldcroft · 2013-09-30T16:40:18Z

@astrofrog - why am I not surprised...

astrofrog · 2013-09-30T16:55:27Z

Well here's the best I can do. I'm stuck at the very last line in the test_table.py file because in that case t._data.__eq__ gets called and I have no control over that, and it bugs. The other way around, I was able to add a workaround. What do you think we should do? The problem is that __array_priority__ won't work because __array__ is defined (c.f. the whole issue with Quantity).

astrofrog · 2013-09-30T16:56:15Z

Also, __eq__ is a little ugly as a result. Why, oh why did I even try testing the masked arrays.

astrofrog · 2013-09-30T16:57:18Z

By the way this is needed because it won't work if you compare a masked to non-masked structured array.

astrofrog · 2013-09-30T17:28:46Z

Failures on Python 2? That's what happens when you develop only in Python 3...

taldcroft · 2013-09-30T18:05:21Z

It seems like there must be a way to simplify the logic in __eq__, but it's not a big deal and might make understanding the behavior even trickier. Otherwise it looks like this is good.

astrofrog · 2013-09-30T18:09:47Z

Will try and simplify things :) What do you think we should do about the failing test?

taldcroft · 2013-09-30T18:15:00Z

About the failed test, I would say to write this case as a known bug and leave it at that for now (and probably put in an astropy issue just so it doesn't get forgotten).

astrofrog · 2013-09-30T19:14:15Z

I tried simplifying the logic, but it's much more readable in its present form.

@taldcroft - if the tests pass, is this good to merge?

taldcroft · 2013-09-30T19:31:17Z

Yes, good to merge.

…arisons Table > value should not work

…arisons Table > value should not work Conflicts: astropy/table/table.py astropy/table/tests/test_table.py

ghost assigned taldcroft Feb 26, 2013

mdboom reviewed Sep 20, 2013
View reviewed changes

astrofrog added 6 commits September 30, 2013 11:28

Added regression test for testing that using comparison operators is …

cb7dd24

…not allowed on tables

Disallow inequality comparisons for Table

846bbcf

Don't override exception in Python 3 (since one already gets raised).…

d91720b

… Also disable equality comparison.

Support equality and added equality tests

652009c

Define Table.__eq__ and Table.__ne__

ad1d85d

Whitespace

658fef6

taldcroft reviewed Sep 30, 2013
View reviewed changes

Get equality to (mostly) work with masked arrays

500c9ee

astrofrog reviewed Sep 30, 2013
View reviewed changes

Fixed tests for Python 2

aedc01b

Removed unused code

71fc2b6

Move failing assertion to an xfail test

03267ec

ghost assigned astrofrog Sep 30, 2013

astrofrog added a commit that referenced this pull request Sep 30, 2013

Merge pull request #828 from astrofrog/table-disallow-inequality-comp…

14a4858

…arisons Table > value should not work

astrofrog merged commit 14a4858 into astropy:master Sep 30, 2013

astrofrog added a commit that referenced this pull request Oct 24, 2013

Merge pull request #828 from astrofrog/table-disallow-inequality-comp…

27b90e8

…arisons Table > value should not work Conflicts: astropy/table/table.py astropy/table/tests/test_table.py

astrofrog deleted the table-disallow-inequality-comparisons branch July 5, 2016 15:53

mcara mentioned this pull request Apr 30, 2019

Make Table.as_array return empty arrays for empty tables #8647

Merged

eerovaher mentioned this pull request Jan 24, 2024

Remove useless Table dunder methods #15945

Merged

1 task done

Conversation

astrofrog commented Sep 20, 2013

Uh oh!

taldcroft commented Feb 27, 2013

Uh oh!

astrofrog commented Feb 27, 2013

Uh oh!

astrofrog commented Sep 20, 2013

Uh oh!

astrofrog commented Sep 20, 2013

Uh oh!

mdboom Sep 20, 2013

Choose a reason for hiding this comment

Uh oh!

astrofrog Sep 20, 2013

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Sep 28, 2013

Uh oh!

taldcroft commented Sep 28, 2013

Uh oh!

taldcroft commented Sep 29, 2013

Uh oh!

taldcroft commented Sep 29, 2013

Uh oh!

mdboom commented Sep 30, 2013

Uh oh!

astrofrog commented Sep 30, 2013

Uh oh!

taldcroft Sep 30, 2013

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Sep 30, 2013

Uh oh!

taldcroft commented Sep 30, 2013

Uh oh!

astrofrog commented Sep 30, 2013

Uh oh!

astrofrog commented Sep 30, 2013

Uh oh!

astrofrog Sep 30, 2013

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Sep 30, 2013

Uh oh!

taldcroft commented Sep 30, 2013

Uh oh!

astrofrog commented Sep 30, 2013

Uh oh!

taldcroft commented Sep 30, 2013

Uh oh!

astrofrog commented Sep 30, 2013

Uh oh!

taldcroft commented Sep 30, 2013

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants