Skip to content

Conversation

sanjanam1998
Copy link

Comment on lines 956 to 957
and hasattr(values, "freq")
and values.freq is not None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
and hasattr(values, "freq")
and values.freq is not None
and hasattr(values, "freq")
and values.freq is not None

Instead could you isinstance(values, DatetimeIndex) (you might need a local import of DatetimeIndex). Also you can use values.inferred_freq to get the frequency instead

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. values.inferred_freq makes sense!
However, my assumption was that we want to make it more generic and not localise this fix to just DatetimeIndex. Please could you help me understand this?

Copy link
Member

Choose a reason for hiding this comment

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

DatetimeIndex is the only relevant object that requires frequency preservation, so it's OK to special case here

Copy link
Author

Choose a reason for hiding this comment

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

Got it!
I have also added TimedeltaIndex as that was one of the examples given in the issue for frequency preservation. Let me know if that makes sense.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

API/BUG: freq retention in value_counts

2 participants