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
np.ma.arrray(['string', np.ma.masked]) gives FutureWarning after #18116 (breaks astropy) #18425
Things get a little confusing for tests that are run in pytest because when warnings are turned into errors then there appears to be a try/except within numpy which triggers immediate conversion to object:
(test-devdeps) (astropy) ➜ astropy git:(master) python -Werror
Python 3.7.9 (default, Aug 31 2020, 07:22:35)
[Clang 10.0.0 ] :: Anaconda, Inc. on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> np.ma.array(['val', np.ma.masked])
masked_array(data=['val', --],
mask=[False, True],
fill_value='?',
dtype=object)
This looks like an unintended warning to me, and is just a case of a deprecation causing an ugly hack in np.ma (namely, that np.ma.masked is a float64 scalar) to surface.
Hmm, we can avoid the FutureWarning being eaten. I was aware of it, but it is more of a problem then I wanted to admit.
I am not immediately sure what to do about the string + masked combination problem. Sure np.ma.masked should not be a float64, but touching that seems like a rabbit hole.
No, I have not looked into it. I am somewhat hoping that we can create a (silly) Masked DType for this purpose. But even then, I am not sure whether we should consider np.ma.array([np.ma.masked, 3]) returning integers a bug-fix or not.
Next problem is that I may not be able to do that until promotion is settled, which it is not. And even if we go that way, I am unsure if there will be some reason why it "casts" the masked to NaN which could be another can of worms.
On the other hand, for NumPy proper, I think the deprecation is important, so...
@seberg , I'll have to defer to @mhvk and @taldcroft on this. I am mainly asking because of astropy/astropy#11291 and worry that if Numpy releases with this unfixed, it is going to completely fail astropy's CI downstream.
I worry quite a few more packages than astropy use np.ma.masked as a placeholder for a masked entry of any type, but I can see that this is not exactly easy to solve -- and am very much in favour of the general principle that one does not auto-convert numbers to strings, etc.:
We could probably go always to error, arguably there is some faint thoughts that it would be a good idea in general to require a flag for "on promotion error just use object". Its a bit interesting, for example np.ma.array([np.datetime("2017"), np.ma.masked]) already returns an object array.
In any case, I plan to try the "special dtype", so that np.ma.array can call np.core.multiarray._multiarray_umath._discover_array_parameters (or np.asarray maybe) with dtype=IgnoreMasked, the only problem is whether that should also ignore the datetime issue, and change the "length" of the string array (note the U32 even without the 0, which comes from the double precision np.ma.masked).
But, right now figuring out how to implement "promotion" exactly is driving me insane, and that is a prerequisite for such a try.
>>> a = [1.0, np.ma.masked]
>>> b = [np.ma.masked, 'val']
>>> Table([a, b], names=('a', 'b'))
<Table length=2>
a b
float64 str3
------- ----
1.0 --
-- val
Under the hood those are just calling np.ma.array on the list inputs.
@seberg - is your "special dtype" effort going to fix the regression so this continues to work with no astropy code changes?
In the worst case we can always catch that FutureWarning internally in astropy and put this into an astropy bug-fix release that goes out before the next numpy release. This will work as long as numpy leaves the FutureWarning in place instead of converting to an error there.
Yes it should fix the regression (and more, potentially much more). But considering on how shaky ground np.ma is build here (out of need, not choice), I would not be surprised if a sandworm comes and swallows the idea for some reason.
Anyway, hopefully I will have tried this in a few weeks...
OK, I have to admit, that most of the things I said were a wrong... For one thing, that part of promotion is already finished. TL;DR: I have to think about it more, I don't think there is a real clear solution and we should likely just hack something in np.ma, input from anyone interested is appreciated.
We should think about how we can make some of the array-coercion (and other) code easier shareable, e.g. with pandas. And I think it can be achieved probably. But, it is a too big project to solve this small regression probably. And, while I am happy to hack a masked-array special case into the core array-coercion, the DType thoughts themselves are probably in that category as well.
About the DType thoughts:
I looked into the masked-array code and that convinced me that this whole thing is basically taped together with a lot of spit and luck, leaving glaring holes that astropy probably just doesn't care about.
Now, there are two approaches for special DTypes, one would be IgnoreMaMasked as I mentioned. But it would probably require some additional logic in the core. Further, I am not sure it can play with a user provided dtype="S" for example.
The next one is a "pandas style" masked dtype: MaskedDType with instances MaskedDType(original_dtype) (at which point np.ma.masked would map to a special MaskedDType(None)). That certainly works, but we would at the very least require knowledge in the core to refuse the use of such a DType (it is a dtype, but it is a bit unclear what NumPy should do with it). That probably works, but might run into limits pretty quickly, for one it probably has to be a DType factory rather than a single DType (which should not be not a big problem long term).
I used to think of the second way as a method of "marrying" the pandas style system with NumPy DTypes, but right now I am not sure. There is also still the possibility of creating "bitpattern" as opposed to "byte- or bit-mask" style masked array-likes. Which also would require a set of DTypes – and would potentially play easier with NumPy. I am not sure that is a problem as such, although it seems that we at least may need some flags for both in any case: Both make sense, but their combination does not. However, that is more a big-picture eco-system problem :).
See astropy/astropy#11364 for a possible fix in astropy, which I hope will keep our tests passing until such time as numpy changes that FutureWarning into an exception. 🤞 :
Thanks, expect we should just fix it in NumPy. We just have to figure out whether to make it a band-aid "fix most occurrences" style fix in np.ma or a more "proper" fix that digs down into the core...
In a developer meeting we discussed this. The general feeling is that this use case is really not a good use of numpy, and we would prefer not to support it. If that is really difficult for astropy, and there is a large demand for this we could reconsider, but we would prefer that astropy actually not create these types of scalar/masked arrays.
we would prefer that astropy actually not create these types of scalar/masked arrays.
@mattip - I'm confused about what that means. At face value that sounds like you don't want astropy to ever create masked arrays of strings, but maybe you really mean that you don't want to support initializing a masked array of strings using ['string', np.ma.masked]? We can live without that particular initialization syntax, though it will cause some pain on our end one way or another.
I'm assuming this will all continue to work:
In [1]: np.ma.array(['string', 'hello'], mask=[False, True])
Out[1]:
masked_array(data=['string', --],
mask=[False, True],
fill_value='N/A',
dtype='<U6')
In [2]: arr = np.ma.array(['string', 'hello'])
In [3]: arr[1] = np.ma.masked
In [4]: arr
Out[4]:
masked_array(data=['string', --],
mask=[False, True],
fill_value='N/A',
dtype='<U6')
Yeah, that is the the thing. The problem is only really with np.ma.masked.
Well the consensus was that we don't really want to add special handling into NumPy core, considering how broken np.ma is. (Special DTypes or similar might be a solution, but right now it would be a very rushed one, and we need better idea around NA+DTypes probably.)
We could try to solve it in np.ma.array(), but considering how broken masked arrays are here, it seemed that it may be more reasonable to work around in astropy :(. But... That does still depend on how awkward it is for astropy, I do not think we are opposed to modifying np.ma.array() if that is easier. (But there also may be reasons why its much less awkward for you, e.g. you may know that all input is 1-D)
Yeah, that is the the thing. The problem is only really with np.ma.masked.
@seberg - again (with apologies), can you be a little more clear for me about what precisely that statement means from the astropy perspective? I am not so much interested in numpy internals or the implementation of possible fixes in numpy. I just want to know what calling syntax and functionality will continue to work without warnings and what will not be supported.
Sorry: yes. Those example would work, things are only problematic if they give a FutureWarning right now. Aside from the initial creation of the masked array, I do not think there is much change. Things like string_masked_array += np.ma.masked would in theory change, but in practice never worked anyway.
The one thing I can think of that might break is: np.ma.concatenate([arr, [np.ma.masked]]), which maybe be something to look into at least providing a work-around (similar to what concatenate has now).
If you know that the result is a string masked array, passing dtype="S" will continue to give the same result, but... its probably hard to know in practice :(.
NumPy master will now always raise the warning when turned into an error. Hopefully, this behaviour is preferential; Of course it doesn't actually fix the problem itself...
I am considering trying gh-19101, which could actually provide a path forward here as well... That would allow to opt-in to fall-back to object. OTOH, that isn't actually the current behaviour probably, so...
I got a little lost in the discussion, but the original issue (future warning, when run with -WError the error causes the fill value to change) has been solved by not raising a FutureWarning. Closing. Please reopen or open a new issue if I misunderstood.
Activity
eric-wieser commentedon Feb 16, 2021
This looks like an unintended warning to me, and is just a case of a deprecation causing an ugly hack in
np.ma(namely, thatnp.ma.maskedis a float64 scalar) to surface.seberg commentedon Feb 16, 2021
Hmm, we can avoid the FutureWarning being eaten. I was aware of it, but it is more of a problem then I wanted to admit.
I am not immediately sure what to do about the
string + maskedcombination problem. Surenp.ma.maskedshould not be afloat64, but touching that seems like a rabbit hole.pllim commentedon Feb 26, 2021
Hello. Is there a time line for this fix? Thanks!
seberg commentedon Feb 26, 2021
No, I have not looked into it. I am somewhat hoping that we can create a (silly)
MaskedDType for this purpose. But even then, I am not sure whether we should considernp.ma.array([np.ma.masked, 3])returning integers a bug-fix or not.Next problem is that I may not be able to do that until promotion is settled, which it is not. And even if we go that way, I am unsure if there will be some reason why it "casts" the masked to
NaNwhich could be another can of worms.On the other hand, for NumPy proper, I think the deprecation is important, so...
seberg commentedon Feb 26, 2021
@pllim or are you interested mainly in getting the
FutureWarningat least raised? That I can do quickly, I had even a start on it.pllim commentedon Feb 26, 2021
@seberg , I'll have to defer to @mhvk and @taldcroft on this. I am mainly asking because of astropy/astropy#11291 and worry that if Numpy releases with this unfixed, it is going to completely fail
astropy's CI downstream.mhvk commentedon Feb 26, 2021
I worry quite a few more packages than astropy use
np.ma.maskedas a placeholder for a masked entry of any type, but I can see that this is not exactly easy to solve -- and am very much in favour of the general principle that one does not auto-convert numbers to strings, etc.:Still, maybe for now raise the FutureWarning but ensure the auto-conversion to object never happens?
seberg commentedon Feb 26, 2021
We could probably go always to error, arguably there is some faint thoughts that it would be a good idea in general to require a flag for "on promotion error just use object". Its a bit interesting, for example
np.ma.array([np.datetime("2017"), np.ma.masked])already returns an object array.In any case, I plan to try the "special dtype", so that
np.ma.arraycan callnp.core.multiarray._multiarray_umath._discover_array_parameters(ornp.asarraymaybe) withdtype=IgnoreMasked, the only problem is whether that should also ignore the datetime issue, and change the "length" of the string array (note theU32even without the0, which comes from the double precisionnp.ma.masked).But, right now figuring out how to implement "promotion" exactly is driving me insane, and that is a prerequisite for such a try.
taldcroft commentedon Feb 26, 2021
To reiterate, I'm interested in the case that is specifically highlighted in the astropy docs as a nice feature:
Under the hood those are just calling
np.ma.arrayon the list inputs.@seberg - is your "special dtype" effort going to fix the regression so this continues to work with no astropy code changes?
In the worst case we can always catch that
FutureWarninginternally in astropy and put this into an astropy bug-fix release that goes out before the next numpy release. This will work as long as numpy leaves theFutureWarningin place instead of converting to an error there.seberg commentedon Feb 26, 2021
Yes it should fix the regression (and more, potentially much more). But considering on how shaky ground
np.mais build here (out of need, not choice), I would not be surprised if a sandworm comes and swallows the idea for some reason.Anyway, hopefully I will have tried this in a few weeks...
seberg commentedon Feb 28, 2021
OK, I have to admit, that most of the things I said were a wrong... For one thing, that part of promotion is already finished. TL;DR: I have to think about it more, I don't think there is a real clear solution and we should likely just hack something in
np.ma, input from anyone interested is appreciated.We should think about how we can make some of the array-coercion (and other) code easier shareable, e.g. with pandas. And I think it can be achieved probably. But, it is a too big project to solve this small regression probably. And, while I am happy to hack a masked-array special case into the core array-coercion, the DType thoughts themselves are probably in that category as well.
About the DType thoughts:
I looked into the masked-array code and that convinced me that this whole thing is basically taped together with a lot of spit and luck, leaving glaring holes that astropy probably just doesn't care about.
Now, there are two approaches for special DTypes, one would be
IgnoreMaMaskedas I mentioned. But it would probably require some additional logic in the core. Further, I am not sure it can play with a user provideddtype="S"for example.The next one is a "pandas style" masked dtype:
MaskedDTypewith instancesMaskedDType(original_dtype)(at which pointnp.ma.maskedwould map to a specialMaskedDType(None)). That certainly works, but we would at the very least require knowledge in the core to refuse the use of such a DType (it is a dtype, but it is a bit unclear what NumPy should do with it). That probably works, but might run into limits pretty quickly, for one it probably has to be a DType factory rather than a single DType (which should not be not a big problem long term).I used to think of the second way as a method of "marrying" the pandas style system with NumPy DTypes, but right now I am not sure. There is also still the possibility of creating "bitpattern" as opposed to "byte- or bit-mask" style masked array-likes. Which also would require a set of DTypes – and would potentially play easier with NumPy. I am not sure that is a problem as such, although it seems that we at least may need some flags for both in any case: Both make sense, but their combination does not. However, that is more a big-picture eco-system problem :).
1 remaining item
taldcroft commentedon Mar 3, 2021
See astropy/astropy#11364 for a possible fix in astropy, which I hope will keep our tests passing until such time as numpy changes that
FutureWarninginto an exception. 🤞 :seberg commentedon Mar 3, 2021
Thanks, expect we should just fix it in NumPy. We just have to figure out whether to make it a band-aid "fix most occurrences" style fix in
np.maor a more "proper" fix that digs down into the core...mattip commentedon Mar 10, 2021
In a developer meeting we discussed this. The general feeling is that this use case is really not a good use of numpy, and we would prefer not to support it. If that is really difficult for astropy, and there is a large demand for this we could reconsider, but we would prefer that astropy actually not create these types of scalar/masked arrays.
taldcroft commentedon Mar 11, 2021
@mattip - I'm confused about what that means. At face value that sounds like you don't want astropy to ever create masked arrays of strings, but maybe you really mean that you don't want to support initializing a masked array of strings using
['string', np.ma.masked]? We can live without that particular initialization syntax, though it will cause some pain on our end one way or another.I'm assuming this will all continue to work:
seberg commentedon Mar 11, 2021
Yeah, that is the the thing. The problem is only really with
np.ma.masked.Well the consensus was that we don't really want to add special handling into NumPy core, considering how broken
np.mais. (Special DTypes or similar might be a solution, but right now it would be a very rushed one, and we need better idea around NA+DTypes probably.)We could try to solve it in
np.ma.array(), but considering how broken masked arrays are here, it seemed that it may be more reasonable to work around in astropy :(. But... That does still depend on how awkward it is for astropy, I do not think we are opposed to modifyingnp.ma.array()if that is easier. (But there also may be reasons why its much less awkward for you, e.g. you may know that all input is 1-D)taldcroft commentedon Mar 11, 2021
@seberg - again (with apologies), can you be a little more clear for me about what precisely that statement means from the astropy perspective? I am not so much interested in numpy internals or the implementation of possible fixes in numpy. I just want to know what calling syntax and functionality will continue to work without warnings and what will not be supported.
Specifically are the code block examples I gave above going to continue working without warnings for the foreseeable future?
seberg commentedon Mar 11, 2021
Sorry: yes. Those example would work, things are only problematic if they give a
FutureWarningright now. Aside from the initial creation of the masked array, I do not think there is much change. Things likestring_masked_array += np.ma.maskedwould in theory change, but in practice never worked anyway.The one thing I can think of that might break is:
np.ma.concatenate([arr, [np.ma.masked]]), which maybe be something to look into at least providing a work-around (similar to whatconcatenatehas now).If you know that the result is a string masked array, passing
dtype="S"will continue to give the same result, but... its probably hard to know in practice :(.charris commentedon May 4, 2021
The time for the 1.21.x branch is approaching. What is the current status of this?
DEP: Ensure the string promotion FutureWarning is raised
seberg commentedon May 16, 2021
NumPy master will now always raise the warning when turned into an error. Hopefully, this behaviour is preferential; Of course it doesn't actually fix the problem itself...
seberg commentedon May 26, 2021
I am considering trying gh-19101, which could actually provide a path forward here as well... That would allow to opt-in to fall-back to object. OTOH, that isn't actually the current behaviour probably, so...
mattip commentedon Sep 7, 2026
I got a little lost in the discussion, but the original issue (future warning, when run with
-WErrorthe error causes the fill value to change) has been solved by not raising a FutureWarning. Closing. Please reopen or open a new issue if I misunderstood.