Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DEPR: Deprecate passing range-like arguments to DatetimeIndex, TimedeltaIndex #23919
DEPR: Deprecate passing range-like arguments to DatetimeIndex, TimedeltaIndex #23919
Changes from 2 commits
f8bed85
5d8a107
28e2184
0f75b9d
2c40c3a
d67f87c
afdab5b
8f435ed
4d7c9e2
2e587e3
0469b74
43a52fc
f4e281e
15e6c30
5dc66f3
b931878
07bfc45
e209a81
d6df7a3
eb5d9c5
cc40717
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
why isn't the default of verify_integrity None?
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.
Because by default we do verify the integrity of a passed frequency. Best guess as to initial motivation is that
verify_integrity=False
is kind of likefastpath=True
and was never really intended to be user-facing.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.
right but aren't you deprecating it?
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.
Yes. In the future it will just be set to
verify_integrity=True
at the top of__new__
. (and when the time comes, in the TDA/DTA constructors it will always be True.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.
We still need the variable to exist because there are cases in which we can determine it is not necessary, in which case we can skip a potentially-expensive check.
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 don't recall that (and dont see how it would work), but yah, not passing freq would make verify_integrity unnecessary.
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.
@jbrockmendel Isn't the end goal to actually remove the
verify_integrity
keyword from the Index constructors? (not just keep it with a fixed True value). Because otherwise, why are you deprecatingverify_integrity=False
if we actually want to use it ourselves?In cases that we determine the check is not necessary, we can use
_simple_new
?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.
Eventually verify-integrity will not be in the signature. The first line of the method will just define it to be True.
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, but then as @jreback said, we should put it at None so we can also deprecate the case for somebody setting it to True explicitly.
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
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.
not important, but was just wondering: why are you adding the '#' everywhere?
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.
Mostly for internal consistency (not a big deal, but for grepping purposes). A little bit because I was curious how long it would take before someone asked about it.
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.
we have have the gh-xxxx style as well, slight prefernce for that