Skip to content

BUG: fix datetimeindex repr #53634

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
Merged
merged 40 commits into from
Jul 22, 2023
Merged

BUG: fix datetimeindex repr #53634

merged 40 commits into from
Jul 22, 2023

Conversation

@Simar-B
Copy link
Contributor

@Simar-B Simar-B commented Jun 13, 2023 •
edited
Loading

Slightly modified _is_dates_only in DatetimeIndex as mentioned in the issue.
@Simar-B Simar-B marked this pull request as ready for review June 13, 2023 19:34
@pmlpm1986
Copy link

pmlpm1986 commented Jun 16, 2023

I added some remarks about this PR in: #53470

Unfortunately, I do not know much about DatetimeArrays to be to help.
@Simar-B Simar-B closed this Jun 21, 2023
@Simar-B Simar-B reopened this Jun 21, 2023
@Simar-B
Copy link
Contributor Author

Simar-B commented Jun 29, 2023

@topper-123 could you take a look whenever you get the chance?
topper-123
topper-123 requested changes Jul 1, 2023
View reviewed changes
Copy link
Contributor

@topper-123 topper-123 left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this.

Looks good generally, a question about the heuristics for finding non-day-based indexes.
pandas/tests/indexes/datetimes/test_datetime.py Outdated
tm.assert_numpy_array_equal(result, expected)

def test__is_dates_only_freq_less_than_day(self):
idx = DatetimeIndex(["2012-01-01 00:00:00"], freq=str(60) + "T")
Copy link
Contributor

@topper-123 topper-123 Jul 1, 2023

Choose a reason for hiding this comment

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

add gh number as first line for each test (# GH53634).
Copy link
Contributor

@topper-123 topper-123 Jul 9, 2023

Choose a reason for hiding this comment

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

_is_dates_only is an internal property, so shouldn't be tested. Can you test that the result from repr(idx) is correct instead.

Also can you move the test to pandas/tests/indexes/datetimes/test_formats.py::TestDatetimeIndexRendering::test_dti_repr_short.
Copy link
Contributor Author

@Simar-B Simar-B Jul 12, 2023

Choose a reason for hiding this comment

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

Could you explain a bit on how the tests at pandas/tests/indexes/datetimes/test_formats.py::TestDatetimeIndexRendering::test_dti_repr_short work? There's no assertion so I'm not sure how they evaluate to a pass or a fail.
@Simar-B Simar-B requested a review from topper-123 July 4, 2023 15:56
@pmlpm1986
Copy link

pmlpm1986 commented Jul 14, 2023

@Simar-B, I added some sanity checks of my own in Simar-B#1
topper-123
topper-123 reviewed Jul 15, 2023
View reviewed changes
Copy link
Contributor

@topper-123 topper-123 left a comment

Choose a reason for hiding this comment

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

A few comments still, looks close.
mroeschke
mroeschke reviewed Jul 18, 2023
View reviewed changes
@Simar-B Simar-B requested review from mroeschke and topper-123 July 18, 2023 20:52
topper-123
topper-123 approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@topper-123 topper-123 left a comment

Choose a reason for hiding this comment

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

LGTM. I'll wait for the response from @mroeschke if he has additional comments.
mroeschke
mroeschke reviewed Jul 19, 2023
View reviewed changes
mroeschke
mroeschke reviewed Jul 19, 2023
View reviewed changes
mroeschke
mroeschke reviewed Jul 19, 2023
View reviewed changes
pandas/core/indexes/datetimes.py Outdated

from pandas.io.formats.format import is_dates_only

delta = self.freq.delta if self.freq and hasattr(self.freq, "delta") else None
Copy link
Member

@mroeschke mroeschke Jul 19, 2023

Choose a reason for hiding this comment

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

Suggested change
delta = self.freq.delta if self.freq and hasattr(self.freq, "delta") else None
delta = getattr(self.freq, "delta", None)
pmlpm1986
pmlpm1986 reviewed Jul 20, 2023
View reviewed changes
pandas/tests/indexes/datetimes/test_formats.py
Copy link

@pmlpm1986 pmlpm1986 Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There seem to be no tests for non-daily and non-hourly freq values. I provided a more comprehensive set of tests in Simar-B#1
Copy link
Contributor Author

@Simar-B Simar-B Jul 22, 2023

Choose a reason for hiding this comment

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

Unfortunately right now it is impossible to differentiate a DatetimeIndex where time has been passed vs when time has not been passed (see here). Therefore, nothing more can be done afaik.
Simar-B and others added 2 commits July 22, 2023 12:00
@Simar-B
Fix formatting
64ddebd
@Simar-B @mroeschke
Use getattr
a9d1698 
Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>
@mroeschke mroeschke added Datetime Datetime data dtype Output-Formatting __repr__ of pandas objects, to_string labels Jul 22, 2023
@mroeschke mroeschke added this to the 2.1 milestone Jul 22, 2023
@mroeschke mroeschke merged commit 3c01ce2 into pandas-dev:main Jul 22, 2023
@mroeschke
Copy link
Member

mroeschke commented Jul 22, 2023

Thanks @Simar-B
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Datetime Datetime data dtype Output-Formatting __repr__ of pandas objects, to_string

4 participants

@Simar-B @pmlpm1986 @mroeschke @topper-123