implement astype portion of #24024 by jbrockmendel · Pull Request #24405 · 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
Conversation78 Commits20 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 }})
and accompanying tests
Uses _eadata like #24394
Constitutes ~10% of the diff of #24024, more after that gets rebased.
is this contingent on #24394 ? should that be first?
is this contingent on #24394 ? should that be first?
No, they are independent. They just both implement+use _eadata.
added requested assertion in test_period and passed copy=copy to numpy copy in the DatetimeLikeArrayMixin.astype case where it was obvious. For the rest I'm going to leave that to Tom in #24024 post-rebase (and if this goes through I'll make a PR on his branch to help rebase)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbrockmendel its much simpler if you actually respond to each comment and resolve if you are doing it, otherwise comment.
| # dtype=object to disable inference. But, DTA.astype ignores |
|---|
| # integer sign and size, so we need to detect that case and |
| # just choose int64. |
| dtype = pandas_dtype(dtype) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this is necessary as it already coerces properly, doing it here is very weird.
In [2]: pd.Index([1,2,3],dtype='int32')
Out[2]: Int64Index([1, 2, 3], dtype='int64')
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you address this?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in the last 8 hours, no. May need to wait on Tom to clarify, since all of this was taken from 24024.
(the fact that these things get closer attention in smaller doses reassures me that splitting is a good idea, even if it does cause rebasing hassles in the parent PR)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, sounds ok then.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What’s the question here? Why we do the integer check? Astype ignores the sign and size. I suppose the index constructor just ignores the size?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if you saw the part about uint vs. int.
So I'm just going to decide that the expected behavior for {Datetime,Timedelta,Period}Index.astype("uint{8,16,32,64}") is to return a UInt64Index. That means we can remove this check and just pass new_values through with the original dtype.
@jbrockmendel do you want to do that here? It's not at all tested, and will need a release note.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok by me (of course its weird to do this, but hey)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to do that here? It's not at all tested, and will need a release note.
I tried this, pretty much just deleting ten lines here, and ended up getting two failures in pandas/tests/indexes/interval/test_astype.py. I can fix this by changing dtype=dtype to dtype=new_values.dtype in the call that wraps self._eadata.astype. Is that what you have in mind?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempt #2 at this also failed. Any other ideas?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this note last night. I implemented this in 3fca810 if you could take a look.
What are the new parts that was discovered in this PR?
What are the new parts that was discovered in this PR?
Primarily the int32 casting stuff and some copy-related topics that I think have been resolved.
| # dtype=object to disable inference. But, DTA.astype ignores |
|---|
| # integer sign and size, so we need to detect that case and |
| # just choose int64. |
| dtype = pandas_dtype(dtype) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having code here is just another inconsistency and maintenance burden. The constructor does the inference.
| @@ -88,24 +87,29 @@ def test_take_raises(): |
|---|
| arr.take([0, -1], allow_fill=True, fill_value='foo') |
| @pytest.mark.parametrize('dtype', [int, np.int32, np.int64]) |
| @pytest.mark.parametrize('dtype', [int, np.int32, np.int64, 'uint']) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of uint will cause 3fca810
to fail. Removing, since it's tested elsewhere now.
Though those tests are just index. I'll add ones for arrays as well...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5fa32e9. The test is slightly more complicated now.
I think
04efd45 broke TimedeltaIndex.astype(str). Looking into it now
Though now that I think about it, this is some pretty strange behavior
In [3]: pd.timedelta_range('2000', periods=2)._data.astype(str)[0] Out[3]: Timedelta('0 days 00:00:00.000002')
Looks like we need to bring in the _format_native_types changes for TimedeltaArray. That should clear all this up.
5d718e6 (hopefully) fixed TImedeltaArray._format_native_types to return an array of strings.
| return self |
|---|
| elif is_period_dtype(dtype): |
| return self.to_period(freq=dtype.freq) |
| return dtl.DatetimeLikeArrayMixin.astype(self, dtype, copy) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed... it'd be nice to leave a bunch of TODO: Use super for places like this.
Actually... I think Python2 will force us to make this changes when we switch inheritance to composition, since we won't be able to call the unbound method with a DatetimeIndex anymore (I think).
This makes the default na repr match the expected type for the formatter.
e29d898 hopefully fixes the Py2 failure. At some point, the type for the na_rep parameter of the datetimelike _format_native_types became unicode. On the base class it's str (on whatever version of Python), and the underlying formatter seems to expect str (again, on whatever version of Python).
lgtm. assume you just rebased? and if you want to add TODO have at it. ping on green.
By "just" rebased do you mean recently? If so I think the last one was
207ffb9.
I'll merge master again when I fix this last py2 error.
If you meant "is rebasing the only thing you did?" then no, there are some real changes here.
It looks like the period tests aren't happy with my changes to format arrays. On 0.23.4 we were inconsistent
In [5]: type(pd.period_range('2000', periods=2).astype(str)[0]) Out[5]: numpy.unicode_
In [6]: type(pd.date_range('2000', periods=2).astype(str)[0]) Out[6]: str
I'll revert the period change here and open an issue.
Merged master and fixed the py2 issue (
a3c42f0).
I decided not to open an issue because the behavior is correct for Python 3 and we're not going to change this for 0.24, so who cares :)
and continue to push changes here if that's OK.
Extremely happy to hand this one off, thanks for figuring it out.
| with pytest.raises(TypeError, match=msg): |
|---|
| idx.astype(dtype) |
| @pytest.mark.parametrize('tz', [None, 'US/Central']) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. This should probably be in indexes/datetimes/test_astype.py. Or rather, we should be using timedelta_range here.
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request