Skip to content

Timestamp rounding wrong implementation #11963

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

Closed
Tux1 opened this issue Jan 5, 2016 · 4 comments
Closed

Timestamp rounding wrong implementation #11963

Tux1 opened this issue Jan 5, 2016 · 4 comments
Labels
Bug Datetime Datetime data dtype Timedelta Timedelta data type
Milestone

Comments

@Tux1
Copy link
Contributor

Tux1 commented Jan 5, 2016

Hi,
I was looking at the recent ENH #4314 and there is a wrong implementation of round method for datetime.

    def round(self, freq):
            """
            return a new Timestamp rounded to this resolution
            Parameters
            ----------
            freq : a freq string indicating the rouding resolution
            """
            cdef int64_t unit
            cdef object result, value

            from pandas.tseries.frequencies import to_offset
            unit = to_offset(freq).nanos
            if self.tz is not None:
                value = self.tz_localize(None).value
            else:
                value = self.value
            result = Timestamp(unit*np.floor(value/unit),unit='ns')
            if self.tz is not None:
                result = result.tz_localize(self.tz)
            return result

You are flooring the nano timestamp instead of rounding. You should replace np.floor by np.round.
Also I propose to add floor and ceil method to Timestamp in order to have the same behavior that float. (It is very usefull to get the upper/lower bound for a specific freq, likely more usefull than round)

Do you agree @jreback ?

@jreback
Copy link
Contributor

jreback commented Jan 5, 2016

hmm, though looks right. Will need some specific tests that catch this.

I would add addtl functions .ceil & .floor (with the same signature). You can of course re-use the impl completely. (just subst the functions). (maybe move the impl to a private ._round which takes the rounding function).

This is used in DatetimeIndex/TimedeltaIndex, Timestamp, Timedelta (the scalars have slightly diff impl).

Just add the this issue number onto the existing note in whatsnew.

@jreback jreback added Bug Datetime Datetime data dtype Timedelta Timedelta data type labels Jan 5, 2016
@jreback jreback added this to the 0.18.0 milestone Jan 5, 2016
@Tux1
Copy link
Contributor Author

Tux1 commented Jan 5, 2016

Yes, I will try to do it

@jreback
Copy link
Contributor

jreback commented Jan 5, 2016

@Tux1 gr8!

also need to add for .dt as well (this is trivial, just add to the accessors list, and test in test_series)

@Tux1
Copy link
Contributor Author

Tux1 commented Jan 5, 2016

yeah, I have looked at your previous commit on that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Datetime Datetime data dtype Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants