You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Empty tables are equal to None but are not None??? #8640
Firstly, is None or == None is not the right thing to do when you want to check if table is empty or not. The proper way is:
>>>t=Table()
>>>len(t) ==0True
Secondly, this behavior is because Table.__eq__ is not the same as is. You can check out the __eq__ implementation in table.py yourself.
>>> t
<Table length=0>
<No columns>
An empty table, as shown above, is clearly not returning None. That is why is None is False. On the other hand, == invokes self.as_array() == other and in your case, t.as_array() is None. Hence, the behavior you are seeing.
as_array(keep_byteorder=False)
Return a new copy of the table in the form of a structured np.ndarray or np.ma.MaskedArray object (as appropriate). table_array:np.ndarray (unmasked) or np.ma.MaskedArray (masked)
Copy of table as a numpy structured array
Documentation for as_array() does not mention that it can return None and under what circumstances it might do so. A regular user does not know that __eq__() is overwritten and calls as_array(). My guess is that this is a bug.
It may have been intentional but I am not sure it is necessarily the right thing to do. I am intuitively expecting as_array() to actually return an array but returning None would be OK if it is impossible to return an empty array for consistency purpose. However, since empty tables are allowed, why would empty arrays not allowed/returned? I am not saying one approach is better than the other but something needs to be done here (docs or code).
Removing if len(self.columns) == 0: return None from as_array() makes as_array() return empty array of raw type. (why not float64 since this is the default type for tables with columns but not data?)
Also the fact that Table() == None is true is just weird. What does it mean?
So, what is finally to be done in order to solve this issue?
If, in case, there is a change to be made to the docs or the code, I would like to work on it.
@disha012 We have to Wait for @taldcroft to suggest what we have to do either changes in docs or code. He is on a vacation so we have to wait for some time.
Hmm, let me pipe in as table co-maintainer: I consider it a bug that Table() == None evaluates to True.
Looking at the cause, I agree with @mcara that it doesn't make much sense to return None in as_array for an empty Table - if the user asked for an array, they should get one! So I think returning an empty array is fine, and I like the suggestion to just remove the if clause altogether, as things will work out that way. (Also a nice example of "Special cases aren't special enough to break the rules", though we should see whether the next one, "Although practicality beats purity", holds...)
@mcara - did you already make the change and want to turn it in a PR? If not, @disha012, if you want to try that, that would be great!
Note that it may be that some Table tests will fail - we will then have to look if it is reasonable to change those, or whether that would cause more trouble than it is worth.
OK, those tests explicitly were for the code not behaving as documented, so I think it is fine to change them - anyway, just left my very brief review; let's move further discussion about the changes there.
Activity
pllim commentedon Apr 25, 2019
Firstly,
is Noneor== Noneis not the right thing to do when you want to check if table is empty or not. The proper way is:Secondly, this behavior is because
Table.__eq__is not the same asis. You can check out the__eq__implementation intable.pyyourself.An empty table, as shown above, is clearly not returning
None. That is whyis NoneisFalse. On the other hand,==invokesself.as_array() == otherand in your case,t.as_array()isNone. Hence, the behavior you are seeing.mcara commentedon Apr 25, 2019
The explanation for the cause is great but is this what is supposed to be? Is this documented? Is this obvious and should not be documented?
From http://docs.astropy.org/en/stable/api/astropy.table.Table.html#astropy.table.Table.as_array:
Documentation for
as_array()does not mention that it can returnNoneand under what circumstances it might do so. A regular user does not know that__eq__()is overwritten and callsas_array(). My guess is that this is a bug.pllim commentedon Apr 25, 2019
Pretty sure returning
Noneis intentional, but good point on clarifying the doc.astropy/astropy/table/table.py
Lines 358 to 359 in d75f07e
mcara commentedon Apr 26, 2019
It may have been intentional but I am not sure it is necessarily the right thing to do. I am intuitively expecting
as_array()to actually return an array but returningNonewould be OK if it is impossible to return an empty array for consistency purpose. However, since empty tables are allowed, why would empty arrays not allowed/returned? I am not saying one approach is better than the other but something needs to be done here (docs or code).Removing
if len(self.columns) == 0: return Nonefromas_array()makesas_array()return empty array of raw type. (why notfloat64since this is the default type for tables with columns but not data?)Also the fact that
Table() == Noneis true is just weird. What does it mean?pllim commentedon Apr 26, 2019
@taldcroft , any input?
disha012 commentedon Apr 28, 2019
So, what is finally to be done in order to solve this issue?
If, in case, there is a change to be made to the docs or the code, I would like to work on it.
geekypathak21 commentedon Apr 28, 2019
@disha012 We have to Wait for @taldcroft to suggest what we have to do either changes in docs or code. He is on a vacation so we have to wait for some time.
mhvk commentedon Apr 29, 2019
Hmm, let me pipe in as table co-maintainer: I consider it a bug that
Table() == Noneevaluates toTrue.Looking at the cause, I agree with @mcara that it doesn't make much sense to return
Noneinas_arrayfor an emptyTable- if the user asked for an array, they should get one! So I think returning an empty array is fine, and I like the suggestion to just remove theifclause altogether, as things will work out that way. (Also a nice example of "Special cases aren't special enough to break the rules", though we should see whether the next one, "Although practicality beats purity", holds...)@mcara - did you already make the change and want to turn it in a PR? If not, @disha012, if you want to try that, that would be great!
Note that it may be that some
Tabletests will fail - we will then have to look if it is reasonable to change those, or whether that would cause more trouble than it is worth.mcara commentedon Apr 29, 2019
I can make a PR but I was waiting for a decision on whether this is a bug or a documentation issue. Thanks for feedback
pllim commentedon Apr 29, 2019
For completeness, those LOCs in questions were originally added in #2790
mhvk commentedon Apr 29, 2019
@pllim - thanks for looking that up. That PR was a pretty major overhaul, so not so surprising that something might have slipped through review.
@mcara - curious to see whether there are any test failures with your suggested change!
mcara commentedon Apr 29, 2019
@mhvk There were 9 total failures in 3 tests due to
assert self.t.as_array() is Nonetests. PR #8647 modified tests accordingly.mhvk commentedon Apr 29, 2019
OK, those tests explicitly were for the code not behaving as documented, so I think it is fine to change them - anyway, just left my very brief review; let's move further discussion about the changes there.