Skip to content

/ pandas Public

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

Jump to bottom

ENH: consistency of input args for boundaries #40245

Open
3 of 8 tasks
attack68 opened this issue Mar 5, 2021 · 15 comments
Open
3 of 8 tasks

ENH: consistency of input args for boundaries #40245

attack68 opened this issue Mar 5, 2021 · 15 comments
Assignees
@hewittk @zyc09
Labels
API - Consistency good first issue Master Tracker Needs Discussion
Milestone
Contributions Wel...

Comments

@attack68
Copy link
Contributor

@attack68 attack68 commented Mar 5, 2021 •
edited by jreback

There are some methods where boundary inputs are required.

This issue proposes standardising to the keyword arg: inclusive and using values {"both", "neither", "left", "right"}.

  • Styler.highlight_between(): 'inclusive' in {"both", "neither", "left", "right"}
  • Series.between() (#40628): 'inclusive' in {"both", "neither", "left", "right"}
  • pd.date_range() #43504
  • DataFrame.between_time() #43248
  • pd.Interval
  • pd.interval_range
  • pd.IntervalIndex
  • pd.cut

More methods yet to be identified.
@hewittk
Copy link
Contributor

@hewittk hewittk commented Mar 5, 2021

take

@hewittk
Copy link
Contributor

@hewittk hewittk commented Mar 17, 2021

Does anyone have the link for the Styler.highlight_between proposed method?

@hewittk
Copy link
Contributor

@hewittk hewittk commented Mar 24, 2021

Does anyone have the link for the Styler.highlight_between proposed method?

Also, are there any other methods or functions that you have in mind?
@attack68

@attack68
Copy link
Contributor Author

@attack68 attack68 commented Mar 24, 2021

#39821 (comment)

so you can see part of the discussion there, that PR is not yet approved, possibly this is a small part of the reason. There is mention of some other functions (non-exhaustive) list that have boundary treatment arguments. Do you know how well numpy handles this, and what style they adopt?

@attack68
Copy link
Contributor Author

@attack68 attack68 commented Mar 24, 2021

one possible solution if it check outs might be to extend Series.between to adopt {both, neither, left, right}, whilst maintaining True is 'both' and False is 'neither' for backwards compatibility.

I also dont think anyone would object to a logical enhancement of the input argument to Series.between.

^just a quick suggestion.

@hewittk hewittk mentioned this issue Mar 25, 2021
Closed
4 tasks
@hewittk hewittk mentioned this issue Mar 25, 2021
Merged
4 tasks
@jreback jreback added this to the 1.3 milestone Mar 29, 2021
@simonjayhawkins simonjayhawkins removed this from the 1.3 milestone Jun 8, 2021
@jreback jreback added this to the 1.3 milestone Jun 16, 2021
@jreback jreback removed this from the 1.3 milestone Jun 17, 2021
@jreback jreback added this to the Contributions Welcome milestone Jun 17, 2021
@jreback jreback removed this from the Contributions Welcome milestone Jun 22, 2021
@jreback jreback added this to the 1.3 milestone Jun 22, 2021
@attack68 attack68 mentioned this issue Jun 27, 2021
Closed
@attack68
Copy link
Contributor Author

@attack68 attack68 commented Jul 2, 2021

Reopening this issue after observing more methods where standardising might be appropriate. (see OP)

@attack68 attack68 reopened this Jul 2, 2021
@simonjayhawkins simonjayhawkins removed this from the 1.3 milestone Jul 2, 2021
@attack68 attack68 mentioned this issue Jul 2, 2021
Merged
@YasmeenAlsaedy
Copy link

@YasmeenAlsaedy YasmeenAlsaedy commented Jul 18, 2021

I would like to take pd.date_range() but can you please explain more what should change in this method @attack68

@suyashgupta01
Copy link
Contributor

@suyashgupta01 suyashgupta01 commented Aug 23, 2021

I want to work on Dataframe.between_time(), is there something else I should know before getting started? @attack68

@suyashgupta01
Copy link
Contributor

@suyashgupta01 suyashgupta01 commented Aug 23, 2021

take

@suyashgupta01
Copy link
Contributor

@suyashgupta01 suyashgupta01 commented Aug 24, 2021 •
edited

Hi @attack68, for Dataframe.between_time(), the boundary related arguments are bool include_start and bool include_end. I'll go ahead and let them stay there for backward compatibility, and add a new inclusive argument. Please let me know if I should adopt another course of action.

Edit: I'll also add a FutureWarning when a user is using the old arguments.

@suyashgupta01
Copy link
Contributor

@suyashgupta01 suyashgupta01 commented Aug 24, 2021

I've made & tested all the aforementioned changes locally, everything seems to be working fine.

Now, I'm aware that I need to write code for tests in the repository before I make a pull request, can anyone please guide me regarding the same.

@suyashgupta01 suyashgupta01 mentioned this issue Aug 27, 2021
Merged
4 tasks
@suyashgupta01
Copy link
Contributor

@suyashgupta01 suyashgupta01 commented Aug 27, 2021 •
edited

@attack68 @jreback
I've made a pull request for DataFrame.between_time(), can you please take a look at it. Also, let me know if any changes are required.

@attack68 attack68 mentioned this issue Sep 4, 2021
Open
@jreback jreback added this to the Contributions Welcome milestone Sep 7, 2021
jreback pushed a commit that referenced this issue Sep 10, 2021
@suyashgupta01
ENH: consistency of input args for boundaries in DataFrame.between_ti…
Verified
This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: 4AEE18F83AFDEB23 Learn about vigilant mode.
Loading status checks…
df1de66 
…me() #40245 (#43248)
@zyc09
Copy link
Contributor

@zyc09 zyc09 commented Sep 10, 2021

take

@zyc09
Copy link
Contributor

@zyc09 zyc09 commented Sep 10, 2021

working on pd.date_range()

@zyc09 zyc09 mentioned this issue Sep 10, 2021
Open
3 tasks
@zyc09 zyc09 mentioned this issue Sep 13, 2021
Merged
4 tasks
@suyashgupta01
Copy link
Contributor

@suyashgupta01 suyashgupta01 commented Sep 14, 2021 •
edited

As mentioned here by @attack68, I'll soon be making a follow up PR for DateTimeIndex.indexer_between_time. :)

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

@hewittk hewittk

@zyc09 zyc09

Labels
API - Consistency good first issue Master Tracker Needs Discussion
Projects
None yet
Milestone
Contributions Welcome
Linked pull requests

Successfully merging a pull request may close this issue.

ENH: New boundary inputs
ENH: Addition of input arguments for boundaries to add more consistency between methods
8 participants
@jreback @simonjayhawkins @attack68 @YasmeenAlsaedy @hewittk @suyashgupta01 @lithomas1 @zyc09