Skip to content
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

Date time index.indexer between time #43602

Conversation

suyashgupta01
Copy link
Contributor

@suyashgupta01 suyashgupta01 commented Sep 16, 2021

  • closes #xxxx
  • tests added / passed
  • Ensure all linting tests pass, see here for how to run them
  • whatsnew entry

Please take a look and let me know if things look fine, I'll make a whatsnew entry then :)

This PR has been made as per this suggestion by @attack68.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

see my comments we need to deprecate

@@ -822,7 +822,7 @@ def indexer_at_time(self, time, asof: bool = False) -> npt.NDArray[np.intp]:
return (time_micros == micros).nonzero()[0]

def indexer_between_time(
self, start_time, end_time, include_start: bool = True, include_end: bool = True
self, start_time, end_time, inclusive="both"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is public api, need to deprecate the inlucde_start/end args.

Copy link
Contributor Author

@suyashgupta01 suyashgupta01 Sep 21, 2021

Choose a reason for hiding this comment

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

Not sure what do I need to do here. Here's what I think I should do:
(1) Add a deprecate directive for old args (include_start, include_end).
(2) Add a line to show deprecation of old args in whatsnew.

An issue with (1) is that the only function calling indexer_between_time is between_time. So, maintaining functionality with old args would just be extra code as we can just modify between_time and pass inclusive instead of include_start, include_end when indexer_between_time is called.

Please let me know what do you think @jreback :)

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. yes
  2. yes

then what you do is you change the call to indexer_between_time`` to use the *new* api (so you don't get a deprecation warning in between_time`

@jreback jreback added the Deprecate Functionality to remove in pandas label Sep 16, 2021
@jreback
Copy link
Contributor

jreback commented Oct 3, 2021

can you merge master and address comments

@suyashgupta01
Copy link
Contributor Author

I'm sorry for the absence! I'd been away due to some work.

@jreback I did leave a comment in my OLD PR addressing why the tests are failing in THIS PR. Can you please take a look at it? I'm not sure what my course of action should be going ahead.

@jreback
Copy link
Contributor

jreback commented Oct 4, 2021

I'm sorry for the absence! I'd been away due to some work.

@jreback I did leave a comment in my OLD PR addressing why the tests are failing in THIS PR. Can you please take a look at it? I'm not sure what my course of action should be going ahead.
you should be able to run this locally and reproduce

@suyashgupta01
Copy link
Contributor Author

suyashgupta01 commented Oct 4, 2021

Yes, indeed I can reproduce the failing test case locally.

However, what I'm confused about is the absence of a code block from pandas/core/generic.py in the master branch.

Can you please let me know why I can see the below code block in this PR, but NOT on the master branch.?

            elif inclusive not in ["both", "neither", "left", "right"]:
                raise ValueError(
                    f"Inclusive has to be either string of 'both', "
                    f"'left', 'right', or 'neither'. Got {inclusive}."
                )

Edit: I'm not sure if this is a bad question but can you please address this @jreback

@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2021

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Nov 5, 2021
@JuompanBR
Copy link

JuompanBR commented Nov 5, 2021 via email

@jreback
Copy link
Contributor

jreback commented Nov 28, 2021

@suyashgupta01 can you merge master and update to comments

@mroeschke
Copy link
Member

Thanks for the PR, but it appears to have gone stale. If you could merge master and address the reviews we would be happy to reopen. Closing.

@mroeschke mroeschke closed this Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants