Skip to content

Change hist(cumulative=-1) to hist(cumulative='reversed') #14650

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

Closed
wants to merge 1 commit into from

Conversation

timhoffm
Copy link
Member

PR Summary

Almost on the limit of being worth it, but IMHO cumulative='reversed' is easier to understand.

@timhoffm timhoffm added this to the v3.2.0 milestone Jun 29, 2019
@anntzer
Copy link
Contributor

anntzer commented Jun 29, 2019

I don't really like the idea (even ignoring backcompat concerns). For example, it is very easy to typo "reversed" into something else that would be considered as True. Sure, you could forbid any strings other than "reversed", but... do we really want to go there?

@timhoffm
Copy link
Member Author

timhoffm commented Jun 29, 2019

I'm not 100% percent sure myself. There are two things:

  1. What do we want the API to look like? Here, cumulative='reversed' is better than cumulative=-1.
  2. How strict are we on the inputs. In particular for bools, we usually accept any values that can be converted to bools (usually just using if param) in the code. That's okish for parameters that have only a boolean meaning (or maybe additionally None). For parmeters that can be bool or 'somestring' I really would not allow any other string inputs, they are almost always typos. - Yes I think we should go there (and for example in subplots(sharey=...) we already do).

@tacaswell
Copy link
Member

Would introducing an enum be a good idea here? You can easily handle either the string or Enum at the top:

In [3]: from enum import Enum

In [4]: class Test(Enum):
   ...:     a = 'a'
   ...:     b = 'b'
   ...: 

In [5]: Test('a')
Out[5]: <Test.a: 'a'>

In [6]: Test.a
Out[6]: <Test.a: 'a'>

In [7]: Test(Test.a)
Out[7]: <Test.a: 'a'>

In [8]: 

and deprecate True / False / -1 ?

Not sure what the enum names should be though...

@timhoffm
Copy link
Member Author

timhoffm commented Jul 2, 2019

See here for my concerns on Enums: #14642 (comment)

We should have a fundamental decision if we want to generally move parameters to Enums or not. That includes a recommendation whether to use them in user code or just have them accepted for better parameter parsing. Accepting both cumutlative='reversed' and cumulative=Cumulative.reversed is fine, but I don't think it would be a good idea to have both equally mixing up in user code.

@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Aug 19, 2019
@timhoffm
Copy link
Member Author

Closing as not generally perceived as an improvement.

@timhoffm timhoffm closed this Oct 29, 2019
@timhoffm timhoffm deleted the hist-cumulative branch October 29, 2019 20:37
@QuLogic QuLogic modified the milestones: v3.3.0, unassigned Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants