Repository navigation
Conversation
This works around what arguably is a bug in numpy < 1.20, and which raises a DeprecationWarning in numpy 1.20-dev. Also has the benefit of the new code being more explicit.
…dtype. This was something that happened to work in numpy < 1.20, but actually led to a Column with arguably the wrong dtype. In numpy 1.20-dev, this leads to DeprecationWarning. Unfortunately, it seems not possible to avoid special-casing the empty list.
|
I thought I saw multiple occurrence of the thingy in votable but I'll double check again to be sure. If nothing is amiss, I'll approve. Thank you so much! |
| # numpy 1.20-dev this is giving a DeprecationWarning. | ||
| # TODO: Adjust when/if the numpy behaviour behaviour changes. | ||
| # Hopefully, this clause can again be removed. NUMPY_LT_1_20. | ||
| self_data = np.empty(0, dtype=dtype) |
There was a problem hiding this comment.
One other solution that is possible if you know that the list represents effectively a 1-D dataset is to use:
np.empty(len(data), dtype=dtype) and then assign to that result. That does have slightly different semantics though.
There was a problem hiding this comment.
Yes, that might indeed work. But I think the standard case should just go through np.array(...), since Column is an ndarray subclass.
The whole thing does make one wonder whether the new behaviour in numpy is in fact an improvement, since it doesn't seem to move to the "more logical" state - re-opening the numpy issue.
|
I've temporarily converted this to a draft, pending further discussion over at the numpy issue. |
|
|
||
| self._base = base | ||
| self.default = np.array([], dtype=self._base.format) | ||
| self.default = np.empty(0, dtype=self._base.format) |
There was a problem hiding this comment.
Looks like this is the only case where [] is used with self._base.format. I think we should wait till Numpy wheel picks up that patch (and hence fails our numpy-dev job on master) and then make sure the numpy-dev job here passes with these changes. Otherwise, LGTM on the io.votable side. Thank you!
|
The failure is on |
|
On Here: |
|
Well, I will try to fix up NumPy to reduce the noise level of this change (for astropy, probably that effectively means no noise), but if the noise comes it might be more annoying :) (but also indicate buggy behaviour). |
|
@seberg , thank you! Please keep us updated. 😄 |
|
@mhvk , does this mean we can close this without merge? |
|
Yes, hopefully, but let's do it only once our numpy-dev tests pass again |
5 participants
fixes #10815
Not very happy with the fix for
Column, but could not think of a better way. Ideally, this gets fixed better by numpy. See numpy/numpy#17511.