BUG: replace coerces incorrect dtype by sinhrks · Pull Request #12780 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation24 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes BUG/API: Fix .replace dtype conversion rules #12747
- tests added / passed
- passes
git diff upstream/master | flake8 --diff - whatsnew entry
| @@ -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 |
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
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)
2. Fix numeric coercion.
3. Add datetime-like tests.
4. Fix datetime-like coercion.
jreback pushed a commit that referenced this pull request
Author: sinhrks sinhrks@gmail.com
Closes #12841 from sinhrks/dtype_tests and squashes the following commits:
e11cc2a [sinhrks] TST: Add numeric coercion tests
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
Powered by Codecov. Last update a62fdf8...f9154e8
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 |
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?
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): |
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?
(WIP)BUG: replace coerces incorrect dtype BUG: replace coerces incorrect dtype
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: |
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)?
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: |
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.
jreback pushed a commit to jreback/pandas that referenced this pull request
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
This was referenced
Mar 20, 2017
jreback added a commit that referenced this pull request
Author: Jeff Reback jeff@reback.net
Closes #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
AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request
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
AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request
mattip pushed a commit to mattip/pandas that referenced this pull request
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
mattip pushed a commit to mattip/pandas that referenced this pull request