Codecov Report

Merging #8647 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8647      +/-   ##
==========================================
- Coverage   86.95%   86.95%   -0.01%     
==========================================
  Files         399      399              
  Lines       59326    59330       +4     
  Branches     1100     1100              
==========================================
  Hits        51588    51588              
- Misses       7097     7101       +4     
  Partials      641      641

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 331767d...b938c12. Read the comment docs.

What about backporting this fix to LTS?

@mcara - did something go wrong in the changelog? Lots of entries unrelated to your have been changed; it would be good to stick to just your item.

@bsipocz - yes, it can likely be backported cleanly, so that would be OK, I think. But it would be good if @taldcroft could have a look...

@mcara - as @bsipocz changed the milestone, the changelog entry is now in the wrong place. Apologies to ask you again to change things, but could you move it down to bug fixes in 2.0.13? (It will automatically be propagated to 3.2 as well.)

Otherwise, this all looks good. I always really like it when we reduce the code! But one further favour: do also add a test that we will not get back your original problem! I.e.,

# Regression test for gh-8640
assert Table() != None

@mhvk Great idea to test Table() == None. There are several issues here. First for empty tables this crashes because of

Specifically, the issue is with the current implementation of __eq__ (I will describe this in a moment) which leads me to the following test:

assert isinstance(t.as_array(), np.ndarray)

instead of Table() == None.

Would that be OK?

With regard to Table.__eq__()... IMO this is a separate issue from the current handling of empty tables and should be dealt separately. However, let me describe what I have observed here.

Empty tables:

In [44]: t = Table(data=None, masked=True)                                                                                      

In [45]: t                                                                                                                      
Out[45]: 
<Table masked=True length=0>
<No columns>

In [46]: t == None                                                                                                              
/.../lib/python3.6/site-packages/astropy-4.0.dev24522-py3.6-macosx-10.12-x86_64.egg/astropy/table/table.py:2764: FutureWarning:
elementwise comparison failed; returning scalar instead,
but in the future will perform elementwise comparison
  result = self.as_array() == other
/.../lib/python3.6/site-packages/astropy-4.0.dev24522-py3.6-macosx-10.12-x86_64.egg/astropy/table/table.py:2764:
FutureWarning: elementwise == comparison failed and returning scalar instead;
this will raise an error or perform elementwise comparison in the future.
  result = self.as_array() == other
Out[46]: array([], dtype=bool)

For empty arrays __eq__ seem to perform element-wise comparison on empty arrays and produces an empty boolean array.

Non-empty Tables. Comparison to tables

In [50]: t = Table(data=[[1,4], [3, 1.0]])                                                                                      

In [51]: t == t                                                                                                                 
Out[51]: array([ True,  True])

This seem to work "as expected" (about this later).

For Non-empty Tables. Comparison to scalars

In [55]: t == 1                                                                                                                 
/.../lib/python3.6/site-packages/astropy-4.0.dev24522-py3.6-macosx-10.12-x86_64.egg/astropy/table/table.py:2764:
FutureWarning: elementwise == comparison failed and returning scalar instead;
this will raise an error or perform elementwise comparison in the future.
  result = self.as_array() == other
Out[55]: False

So, here we can see that comparison with scalars does not work. It must be done column-by-column.

• So, in the future, these comparisons to scalars will not work.
• Comparison of empty tables to a scalar (or None) will produce an empty array. Also, note: np.any([]) (or all) is True according to this comment.

Equality for astropy tables was implemented as comparison of structured arrays. IMO this was a mistake and ad hoc implementation of equality test. This implementation is of very limited usefulness. For example. even such an obvious comparison fails:

In [58]: Table(data=[[1,4], [3, 1.0]]) == [[1,4], [3, 1.0]]                                                                     
/.../lib/python3.6/site-packages/astropy-4.0.dev24522-py3.6-macosx-10.12-x86_64.egg/astropy/table/table.py:2764: FutureWarning:
elementwise == comparison failed and returning scalar instead;
this will raise an error or perform elementwise comparison in the future.
  result = self.as_array() == other
Out[58]: False

I would have hoped that this will actually compare the elements of the table.

By comparison, pandas has a much more useful implementation for equality test - see https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.equals.html:

In [1]: import pandas as pd                                                                                   

In [2]: df = pd.DataFrame({1: [10], 2: [20]})                                                                 

In [3]: df == 10                                                                                              
Out[3]: 
      1      2
0  True  False

But this is a bigger/larger discussion to have and IMO it deserves its own issue.

@mhvk Would the following test suffice?

        # Regression test for gh-8640
        assert isinstance(self.t == None, np.ndarray)
        assert (self.t == None).size == 0

By the way, even a test like this one:

In [2]: Table(masked=True).as_array() == None                                                                                   
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-2-7c0f737df615> in <module>
----> 1 Table(masked=True).as_array() == None

~/.../lib/python3.6/site-packages/numpy/ma/core.py in __eq__(self, other)
   4043         and other considered equal only if both were fully masked.
   4044         """
-> 4045         return self._comparison(other, operator.eq)
   4046 
   4047     def __ne__(self, other):

~/.../lib/python3.6/site-packages/numpy/ma/core.py in _comparison(self, other, compare)
   3996             # Now take care of the mask; the merged mask should have an item
   3997             # masked if all fields were masked (in one and/or other).
-> 3998             mask = (mask == np.ones((), mask.dtype))
   3999 
   4000         else:

ValueError: No fields found.

In [3]: Table(masked=False).as_array() == None                                                                                  
/.../bin/ipython:1: FutureWarning: elementwise == comparison failed and returning scalar instead;
this will raise an error or perform elementwise comparison in the future.
Out[3]: False

does not work consistently: works for regular arrays but not for masked arrays.

xref: numpy/numpy#13438

@mhvk By the way, adding back

        if len(self.columns) == 0:
            return empty_init(0)

makes Table(masked=True) == None not to crash anymore (but still with numpy warnings).

We probably can fix the crash also by making the final array creation be empty_init(len(self), dtype=dtype or None) (this since bool([]) evaluates to False)

@taldcroft - do you want to have a quick look here? It looks OK to me.

@mcara - I missed the details: what in the end went wrong with just removing the if clause altogether? (I.e., with my suggestion in the comment preceding this one?)

@mcara - this looks good, thanks for getting into this and fixing this inconsistency. The code and tests look fine, my only concern now is about the scope of the change.

To me this really looks like an API change since the output type is now different for a relatively simple operation that might occur in practice (Table().as_array()). So there could be code out there using LTS which expects None. The original output of None was not a bug, it was a design decision that we now agree should be changed.

So I would propose moving this back to 3.2 and calling it an API change.

In addition, the fix which now allows Table([]) to create an empty table instead of crashing should be documented separately in the changelog as a bug fix.

@mcara - regarding comparisons, thanks for bringing some fresh perspective. Can you open a new issue with some of the examples you have put here so we can discuss this in a clean issue?

One quick note on a difference between Table and pandas. Astropy Table supports multi-dimensional columns unlike pandas, so the example you gave below could be interpreted as asking for an element-by-element comparison to the right-hand-side 2x2 array. So you need to keep this in mind when asking how astropy Table should behave.

Table(data=[[1,4], [3, 1.0]]) == [[1,4], [3, 1.0]]

Given feature freeze for 3.2 is gone, I am milestoning this to 3.2.1 instead, but @bsipocz can always re-milestone back to 3.2 if she so desires. I agree that this is more of an API change than a bug fix.

Given it has the API change (but in fact more like a bugfix with an API change), I would be more comfortable to ship it in the main 3.2 release rather than in a bugfix.

Sorry for the delay in answering.

If you do not specify the type and leave it to an empty list [], then you will have several tests failing. Here is one example:

_________________________________________________________________________________ TestRemove.test_delitem1[masked] _________________________________________________________________________________

self = <astropy.table.tests.test_table.TestRemove object at 0x136100978>, table_types = <astropy.table.tests.conftest.table_types.<locals>.TableTypes object at 0x136100e48>

    def test_delitem1(self, table_types):
        self._setup(table_types)
        del self.t['a']
        assert self.t.columns.keys() == []
        assert self.t.as_array().size == 0
        # Regression test for gh-8640
        assert not self.t
>       assert isinstance(self.t == None, np.ndarray)

astropy/table/tests/test_table.py:958:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
astropy/table/table.py:2755: in __eq__
    result = (self.as_array().data == other) & (self.mask == false_mask)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <Table length=0>
<No columns>, other = array([()], dtype=[])

    def __eq__(self, other):

        if isinstance(other, Table):
            other = other.as_array()

        if self.masked:
            if isinstance(other, np.ma.MaskedArray):
                result = self.as_array() == other
            else:
                # If mask is True, then by definition the row doesn't match
                # because the other array is not masked.
                false_mask = np.zeros(1, dtype=[(n, bool) for n in self.dtype.names])
                result = (self.as_array().data == other) & (self.mask == false_mask)
        else:
            if isinstance(other, np.ma.MaskedArray):
                # If mask is True, then by definition the row doesn't match
                # because the other array is not masked.
                false_mask = np.zeros(1, dtype=[(n, bool) for n in other.dtype.names])
                result = (self.as_array() == other.data) & (other.mask == false_mask)
            else:
>               result = self.as_array() == other
E               ValueError: No fields found.

astropy/table/table.py:2763: ValueError

@taldcroft

So there could be code out there using LTS which expects None. The original output of None was not a bug, it was a design decision that we now agree should be changed.

I probably disagree with the first point: we have arrived to this issue by accident and I doubt people would use do this kind of comparisons such as Table() == None. Most likely people would do if table: # then it is not empty.

I agree with the second point that this is not a bug. However, as you mention Table([]) is a bug fix (CC: @bsipocz) and it is included in this PR. Should I split this PR into two PR? just to replace != 1 with > 1 in one line? I do not have any strong opinion here, just saying.

Should I split this PR into two PR?

No. I assume this will be wrapped up in a few days, so getting it into the RC won't be an issue.

So, do I move the changelog entry? to what version?

To the bugfix section of 3.2, and another entry for the API change section of 3.2. I think. But @taldcroft might chime in.

I am not familiar with the procedures here, I got confused by #8647 (comment) and mentioning of 3.2.1.

@mcara - We're in feature freeze for 3.2. But the release is not out and bugfixes can be still added to it. We started to 3.2.1 milestone PRs that are expected to be finished after 3.2 is out or that are non blockers for the release.
Nominally API changes should never land in a bugfix release, so if we're very strict this can only be shipped in 4.0. But there seems to be an agreement that this would be good to be included in 3.2.

I probably disagree with the first point: we have arrived to this issue by accident and I doubt people would use do this kind of comparisons such as Table() == None. Most likely people would do if table: # then it is not empty.

Core astropy is a big project and I generally avoid trying to guess what people will do (and they will often surprise you!!). Instead just make a straight judgement on whether API is changing and in this case that is unambiguous.

To the bugfix section of 3.2, and another entry for the API change section of 3.2. I think.

• The fact that Table([]) raised an exception instead of an empty table like Table() is a bug.
• The change of Table().as_array() from None to an empty array is an API change.

Note: CircleCI mpl failures seem unrelated. Please move the change log as @bsipocz suggested. Sorry for the confusion. Thanks!

I agree what seems the consensus above: merge this for 3.2, but don't backport.

@taldcroft or @mhvk , please approve if you are happy with this now. Thanks!

Thanks for reviewing this issue and PR!