API: Disallow NaN in StringArray constructor #30980
Conversation
TomAugspurger
added
ExtensionArray
pandas/_libs/lib.pyx
Outdated
|
|
||
| def __cinit__(self, Py_ssize_t n, dtype dtype=np.dtype(np.object_), | ||
| bint skipna=False): | ||
| bint skipna=False, | ||
| bint na_only=False): |
There was a problem hiding this comment.
Alternatively, if people don't like this, I can make a new validator that's specific to StringArray's checking.
class StringArrayValidator(StringArray):
cdef bint is_valid_null(self, object value) except -1:
return value is C_NADo people have a preference?
|
Actually, we don't use |
pandas/core/arrays/numpy_.py
Outdated
| return cls(result) | ||
|
|
||
| @staticmethod | ||
| def _from_sequence_finalize(values, copy): | ||
| return values |
There was a problem hiding this comment.
Why is the different name needed, and can't it just be _from_sequence?
There was a problem hiding this comment.
This lets StringArray customize a bit of the logic in the middle of _from_sequence. Without this StringArray._from_sequence raised on the super()._from_sequence, since the init would raise. This lets us still reuse PandasArray._from_sequence.
Happy to have suggestions for a better name. I didn't put any thought into this one. _finalize isn't the best since it's not happening after _from_sequence, it's in the middle.
There was a problem hiding this comment.
So the call chain is roughly
StringArray._from_sequnce
-> PandasArray._from_sequnce (via the super)
-> StringArray._from_sequnce_finalize
-> StringArray._from_sequence (after the super finishes)
hopefully that makes sense.
There was a problem hiding this comment.
Ah, I didn't notice that it still called super in the middle, understand now.
But, I would personally just not call super and implement all in StringArray._from_sequence. Yes, this gives a little bit of duplication (but really not a lot IMO), but I think that is better than the additional method name and harder to understand call chain.
There was a problem hiding this comment.
Renamed to _coerce_from_sequence_values
There was a problem hiding this comment.
OK, I've inlined things. Apologies for the force push, I'm not sure what happened.
|
|
||
|
|
||
| def test_from_sequnce_no_mutate(): | ||
| a = np.array(["a", pd.NA], dtype=object) |
There was a problem hiding this comment.
use np.nan or None here? Otherwise the original won't be different from the potentially mutated one?
There was a problem hiding this comment.
Whoops, yes. That's what I intended.
TomAugspurger
force-pushed
the
30966-str-validate
branch
from
b34b600 to
bbe2196
Compare
jorisvandenbossche
changed the title
|
thanks @TomAugspurger |
) Co-authored-by: Tom Augspurger <[email protected]>
Closes #30966
There were a few ways we could have done this. I opted for this way since it still only requires a single pass over the data to validate the values. Other ways like checking fornp.isnanafter checkingis_string_arraywould have required a second pass.I changed the implementation in subsequent commits. The basic idea is the same, don't allow NaN in the array passed to StringArray, so that we only make a single pass over the data. We do this by changing
StringValidator.is_valid_nullto only allow NA. This required a small change toPandasArray._from_sequence, since previously we relied on creating a temporarily invalid StringArray before doing an inplace__setitem__to replace NaNs with NA.cc @tsvikas.