-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: replace coerces incorrect dtype #12780
Conversation
8ca4b9a
to
df2eae1
Compare
@@ -1355,6 +1365,12 @@ class NumericBlock(Block): | |||
is_numeric = True | |||
_can_hold_na = True | |||
|
|||
def _can_replace_element(self, element): | |||
# coercion is handled by putmask | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
In [1]: issubclass(np.bool_, np.number)
Out[1]: False
In [2]: issubclass(np.float_, np.number)
Out[2]: True
72d13eb
to
0c7f4de
Compare
Found this affects more functions than I thought. Let me do it in 4 separate PRs. 1 Add numeric related tests to clarify the current behaviour (and future change) |
Current coverage is 85.97% (diff: 89.65%)@@ master #12780 diff @@
==========================================
Files 140 140
Lines 51254 51275 +21
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 44068 44085 +17
- Misses 7186 7190 +4
Partials 0 0
|
after #13147 this should be slightly more standard |
|
||
def _assert_replace_conversion(self, from_key, to_key, how): | ||
index = pd.Index([3, 4], name='xxx') | ||
obj = pd.Series(self.rep[from_key], index=index, name='yyy') | ||
self.assertEqual(obj.dtype, from_key) | ||
|
||
if (from_key.startswith('datetime') and to_key.startswith('datetime')): | ||
# different tz, currently mask_missing raises SystemError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this for another issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, issued #15183. Let me fix it in a separate pr.
@sinhrks looks pretty good, just a couple of small comments. |
@@ -3233,6 +3236,16 @@ def comp(s): | |||
return _possibly_compare(values, getattr(s, 'asm8', s), | |||
operator.eq) | |||
|
|||
def _cast(block, scalar): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might be a more general routine (IOW maybe defin as a utility function in internals rather than a nested function here), or maybe as a block method?
cast_scalar
?
@sinhrks status? (lgtm so far) |
Updated to fix the above points, and now ready for review. |
looks fine. still some internal things ideally should clean up. but can do later. though couple of comments if you can address. |
@@ -361,6 +377,11 @@ def _infer_dtype_from_scalar(val): | |||
elif is_complex(val): | |||
dtype = np.complex_ | |||
|
|||
elif pandas_dtype: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this parameter necessary, why wouldn't we always want to infer a pandas dtype if its there (e.g. Period
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed for the PR itself. The same func is required for #14145.
if val is tslib.NaT or val.tz is None: | ||
dtype = np.dtype('M8[ns]') | ||
else: | ||
if pandas_dtype: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. here, is this a back-compat issue?
@sinhrks I am rebasing this and will merge after testing. |
closes pandas-dev#12747 Author: sinhrks <sinhrks@gmail.com> This patch had conflicts when merged, resolved by Committer: Jeff Reback <jeff@reback.net> Closes pandas-dev#12780 from sinhrks/replace_type and squashes the following commits: f9154e8 [sinhrks] remove unnecessary comments 279fdf6 [sinhrks] remove import failure de44877 [sinhrks] BUG: replace coerces incorrect dtype
closes pandas-dev#12747 Author: sinhrks <sinhrks@gmail.com> This patch had conflicts when merged, resolved by Committer: Jeff Reback <jeff@reback.net> Closes pandas-dev#12780 from sinhrks/replace_type and squashes the following commits: f9154e8 [sinhrks] remove unnecessary comments 279fdf6 [sinhrks] remove import failure de44877 [sinhrks] BUG: replace coerces incorrect dtype
xref pandas-dev#15736 xref pandas-dev#12780 Author: Jeff Reback <jeff@reback.net> Closes pandas-dev#15765 from jreback/common_types and squashes the following commits: d472646 [Jeff Reback] try removing restriction on windows 8d07cae [Jeff Reback] CLN: replace _interleave_dtype with _find_common_type
closes pandas-dev#12747 Author: sinhrks <sinhrks@gmail.com> This patch had conflicts when merged, resolved by Committer: Jeff Reback <jeff@reback.net> Closes pandas-dev#12780 from sinhrks/replace_type and squashes the following commits: f9154e8 [sinhrks] remove unnecessary comments 279fdf6 [sinhrks] remove import failure de44877 [sinhrks] BUG: replace coerces incorrect dtype
xref pandas-dev#15736 xref pandas-dev#12780 Author: Jeff Reback <jeff@reback.net> Closes pandas-dev#15765 from jreback/common_types and squashes the following commits: d472646 [Jeff Reback] try removing restriction on windows 8d07cae [Jeff Reback] CLN: replace _interleave_dtype with _find_common_type
git diff upstream/master | flake8 --diff