Skip to content

TST: Add regression tests for concat with non-ns DatetimeIndex units (GH#58471) #62304

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

TST: Add regression tests for concat with non-ns DatetimeIndex units (GH#58471) #62304

wants to merge 1 commit into from

Conversation

@skalwaghe-56
Copy link
Contributor

@skalwaghe-56 skalwaghe-56 commented Sep 9, 2025 •
edited
Loading

Adds regression tests for pandas issue #58471. Added a total of 3 tests. All tests are parameterized across datetime units (ns/us/ms/s) to verify consistent behavior.

Thanks!

@skalwaghe-56
Copy link
Contributor Author

skalwaghe-56 commented Sep 9, 2025

@jbrockmendel I kindly request you to check the PR and let me know!

@jbrockmendel
Copy link
Member

jbrockmendel commented Sep 9, 2025

Is this AI?

@skalwaghe-56
Copy link
Contributor Author

skalwaghe-56 commented Sep 10, 2025

Is this AI?

The description of the PR contains some things from AI. I'm sorry if its an problem.

@skalwaghe-56
Copy link
Contributor Author

skalwaghe-56 commented Sep 10, 2025

Removed the AI part! Sorry for that.

@skalwaghe-56
Copy link
Contributor Author

skalwaghe-56 commented Sep 10, 2025

@jbrockmendel Please check it now. Thanks!

@simonjayhawkins simonjayhawkins added the Testing pandas testing functions or related to the test suite label Sep 10, 2025
@skalwaghe-56 skalwaghe-56 force-pushed the fix-issue-58471 branch 2 times, most recently from d9f5c25 to 2cd8c3a Compare September 11, 2025 14:30
@skalwaghe-56
Copy link
Contributor Author

skalwaghe-56 commented Sep 11, 2025

@rhshadrach Request you to kindly check this.

@jbrockmendel
Copy link
Member

jbrockmendel commented Sep 11, 2025

@skalwaghe-56 you don't need to ping us on every PR. We'll get to them all in due time.

@skalwaghe-56
Copy link
Contributor Author

skalwaghe-56 commented Sep 11, 2025

@jbrockmendel Sorry Sir!

@skalwaghe-56 skalwaghe-56 force-pushed the fix-issue-58471 branch 5 times, most recently from ffe118d to 5650319 Compare September 17, 2025 16:56
simonjayhawkins
simonjayhawkins requested changes Sep 19, 2025
View reviewed changes
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @skalwaghe-56 for the PR.

Let's not add all these tests, just the minimum reproducible example, which in the linked issue looks like took a few contributors until it converged on anything that look like one!

@skalwaghe-56
Copy link
Contributor Author

skalwaghe-56 commented Sep 19, 2025

@simonjayhawkins Thanks for your review! I've confirmed that we include just the minimum reproducible example. I've added 2 test which I think are necessary for this issue and to confirm that is fixed properly. Thanks!

rhshadrach
rhshadrach requested changes Sep 20, 2025
View reviewed changes
@rhshadrach rhshadrach added Needs Tests Unit test(s) needed to prevent regressions Datetime Datetime data dtype Reshaping Concat, Merge/Join, Stack/Unstack, Explode and removed Testing pandas testing functions or related to the test suite labels Sep 20, 2025
jbrockmendel
jbrockmendel reviewed Sep 22, 2025
View reviewed changes
@skalwaghe-56 skalwaghe-56 force-pushed the fix-issue-58471 branch 5 times, most recently from 431fa2f to cfa7fd3 Compare September 30, 2025 10:57
@skalwaghe-56 skalwaghe-56 force-pushed the fix-issue-58471 branch 3 times, most recently from 8cb6204 to a0dd9e7 Compare October 4, 2025 06:28
rhshadrach
rhshadrach requested changes Oct 4, 2025
View reviewed changes
@skalwaghe-56 skalwaghe-56 force-pushed the fix-issue-58471 branch 2 times, most recently from 01e5f63 to caf584d Compare October 6, 2025 01:36
rhshadrach
rhshadrach requested changes Oct 26, 2025
View reviewed changes
@skalwaghe-56 skalwaghe-56 force-pushed the fix-issue-58471 branch 2 times, most recently from b7b54f1 to c9d8049 Compare November 1, 2025 11:56
rhshadrach
rhshadrach approved these changes Nov 1, 2025
View reviewed changes
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rhshadrach
Copy link
Member

rhshadrach commented Nov 1, 2025

@simonjayhawkins - you good here?

jbrockmendel
jbrockmendel reviewed Nov 3, 2025
View reviewed changes
pandas/tests/reshape/concat/test_datetimes.py
result = concat([first, second])
tm.assert_frame_equal(result, expected)

@pytest.mark.parametrize("unit", ["ns", "us", "ms", "s"])
Copy link
Member

@jbrockmendel jbrockmendel Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a fixture for this. OK for follow-up

Copy link
Contributor Author

@skalwaghe-56 skalwaghe-56 Nov 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find an existing fixture. Thanks!

Copy link
Member

@rhshadrach rhshadrach Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unit, you can just remove the @pytest.mark.parametrize line.

pandas/pandas/conftest.py

Line 1249 in 607e489

def unit(request):

Copy link
Contributor Author

@skalwaghe-56 skalwaghe-56 Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@rhshadrach rhshadrach rhshadrach approved these changes

@simonjayhawkins simonjayhawkins Awaiting requested review from simonjayhawkins

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

Datetime Datetime data dtype Needs Tests Unit test(s) needed to prevent regressions Reshaping Concat, Merge/Join, Stack/Unstack, Explode

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BUG: concatenating time series with non ns unit leads to dataframe with missing data

4 participants

@skalwaghe-56 @jbrockmendel @rhshadrach @simonjayhawkins