-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
TST: Period with Timestamp overflow #34755
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
Conversation
| ) | ||
| period.start_time | ||
| period.end_time | ||
| period = Period( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a separate test function for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in the last PR
| second=bound.second - offset, | ||
| freq="us", | ||
| ) | ||
| period.start_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to assert what this operation returns. assert period.start_time == ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried write a better line for this one, but didn't managed.
|
Hi @OlivierLuG - is this still active? If so, can you address Matt's comments? |
|
I was sure to made answers... I've made a new PR taking into account @mroeschke review. |
| assert isinstance(p1, Period) | ||
| assert isinstance(p2, Period) | ||
|
|
||
| def period_constructor(bound, offset): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just define this in the test since it's only used there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK done
| def test_start_time_and_end_time_bounds(self, bound, offset): | ||
| # GH #13346 | ||
| period = TestPeriodProperties.period_constructor(bound, -offset) | ||
| assert period.start_time.round(freq="S") == period.to_timestamp().round( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you parameterize over start_time and end_time so you can just call getattr(period, time_variable).method(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests were splitted in two tests test_outter_bounds_start_and_end_time and test_inner_bounds_start_and_end_time with a better parametrization.
Thanks for the tips, I didn't knew about getattr function.
|
can u merge master and will look |
|
@OlivierLuG is this still active? |
I'm still here. I'll take a look to this PR on sunday. |
|
can you merge master, i think a lot has changed since this was last rebase (but it still should pass); ping on green. |
|
OK, done and ping on green |
|
Thanks @OlivierLuG |
* #TST pandas-dev#13346 added tests * TST pandas-dev#13346 taken review into account * Added tests for pandas-dev#13346 - with review
* #TST pandas-dev#13346 added tests * TST pandas-dev#13346 taken review into account * Added tests for pandas-dev#13346 - with review
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffThe first part of the test checks that the period inside boundaries does not raise exception when calling
start_timeandend_timemethods. The second part checks that methods raise exceptions when outside of boundaries.The parameters are
Timestamp.minandTimestamp.max, so the tests will follow any change of the boundaries.