-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: .date and timezones #11757
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
Comments
Python |
This I believe is the same as #11800. I have the tests ready to go once apply is changed as mentioned there. |
@rockg did you have a PR for this? |
I have not pushed it...will do so by tomorrow morning. |
gr8 thanks |
I thought this would be fixed by #11564 but it is still around. Was that your understanding?
|
that was much more focused on timeseltas |
All right...I need some direction then. This is the problem spot. Basically it gets the datetime64s out and converts those into Timestamps all of which are tz-unaware. One solution is to remove the tz like |
oh, that's non-conformant code, maybe something like:
|
@rockg Did you have something? |
I think this was actually fixed by some other pull requests (perhaps the referenced issues), but I cannot test for a couple days. If not fixed, then you can push. |
Hmm, not really clear what was actually the issue. This example from @jreback:
Isn't this the correct and expected output? (master still gives the same) The second issues was the
where the UTC dates are given. This seems to be fixed in master:
|
Is s equal to [1] above and in US/Eastern? If so, then master is now correct. Before it was converting under the hood to UTC and then taking date of that which was wrong. |
Yes, I used the |
Yes, [3] is right. There cannot be timezones attached to date objects. For me the real issue was that |
And that issue is now fixed so I think this can be closed. |
I don't think this is fixed |
I always understood |
that's right they should be datetime.datetime WITH a tzinfo |
they r missing the tzinfo |
But I'm saying they should by python date objects so without a timezone. |
Just like if you have a datetime.datetime and call |
no that doesn't make sense - u have lost the tz for no reason (this is of course for a tz aware series - obviously a tz naive one this is already correct) |
try date on a tz-aware datetime.datetime |
|
And raw datetime:
|
so the issue is that we alway have |
@jreback So you propose that Of course such a change could be discussed, but I agree with @rockg that I also don't see the purpose of this change. If we do not change it, this issue indeed only needs some tests to confirm the fixed apply behaviour to get closed. |
no this is fine though maybe should doc it somehow/where |
I'll add tests/doc mention tonight and then we should be good. |
from SO
These are correct as they are in the local timezone, but we are not propogating the tz here and just returning a naive date (in local zone)
These are converted to UTC, then returning a naive date
These should be local & with a timezone attached
further, test
s.apply(lambda x: x)
The text was updated successfully, but these errors were encountered: