BUG: Fix inserting of wrong-dtyped NaT, closes #27297 by jbrockmendel · Pull Request #27311 · 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 Commits19 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: DatetimeArray/Series incorrectly accepts timedelta64('NaT') for __setitem__ #27297
- tests added / passed
- passes
black pandas - passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
Needs tests for PeriodDType case. The tests are probably not in the ideal place, how to move/parametrize them depends on what axis we want to sort them along (i.e. all Series indexing tests together or all timedelta-insertion tests together)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this patch is very awkward. This needs work to make it more fluid.
this patch is very awkward. This needs work to make it more fluid.
Might be because it addresses three separate-but-related bugs. Looking at it commit-by-commit might make more sense. I'm happy to make separate PRs if that's easier
This was referenced
Jul 10, 2019
Rebased following #27323. This does for datetime64 Series what that did for timedelta64 Series
no whatsnew, so doesnt need to be moved
| @@ -2585,8 +2585,11 @@ def setitem(self, indexer, value): |
|---|
| try: |
| return super().setitem(indexer, value) |
| except (ValueError, TypeError): |
| obj_vals = self.values.astype(object) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you really need to do this?
if you actually do, then .reshape(1, -1) should just work (or use np.atleast_2d)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this really is needed.
reshape(1, -1) is what is already here
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you give an example that specific needs this 2d reshape?
this just is a completely different pattern than anywhere else. Why for example would the ObjectBlock construct not simply reshape (if needed); it already knows the dim & the values shape?
| @@ -2585,8 +2585,11 @@ def setitem(self, indexer, value): |
|---|
| try: |
| return super().setitem(indexer, value) |
| except (ValueError, TypeError): |
| obj_vals = self.values.astype(object) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you give an example that specific needs this 2d reshape?
this just is a completely different pattern than anywhere else. Why for example would the ObjectBlock construct not simply reshape (if needed); it already knows the dim & the values shape?
Why for example would the ObjectBlock construct not simply reshape (if needed); it already knows the dim & the values shape?
Because the Block constructor calls self._validate_ndim to check that it got the expected shape. reshaping is done earlier.
Why for example would the ObjectBlock construct not simply reshape (if needed); it already knows the dim & the values shape?
Because the Block constructor calls self._validate_ndim to check that it got the expected shape. reshaping is done earlier.
is this not exactly what _block_shape does?
is this not exactly what _block_shape does?
It is, but block_shape gets called before the constructor.
But apparently it isnt necessary. Just ran the tests with this reshape disabled and its fine now. Will remove.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I think this need s a whatsnew note, would do on 1.0
| if not should_cast: |
|---|
| expected = expected.astype(object) |
| ser = base.copy(deep=True) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these need to be deep? (no big deal though)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
quintusdias pushed a commit to quintusdias/pandas_dev that referenced this pull request
2 participants
