-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(python): Rename first qcut
parameter to quantiles
#10253
Conversation
I renamed them because it's a standard name for a parameter that does double duty. The problem is that they are emphatically NOT quantiles. I've had similar discussions about this incorrect naming before. To be clear, I'm not trying to give you a hard time and I only noticed this because it created a conflict with another change. My issue isn't with not wanting to use single letter names (which I think is the policy you're referring to?) but with using inaccurate names. While I value terseness, correctness and descriptiveness come first. EDIT: I thought I was going crazy so I just had to check. I explicitly mentioned the name change in the PR because even though it's standard it is short so I wanted to fish for something better. That part was accepted as-is! |
I didn't go through the commit history here - I just ran into this when updating the deprecation utils. I'm not trying to rock your boat. Indeed, it's harder to adequately name parameters that do 'double duty'. In this case, I think |
I dunno, |
@magarick has a good point. This parameter really specifies the probability level of the (whished for) quantile. The base R function Unfortunately, there is no common naming:
|
@stinodego Why don't you just change it to |
The problem is one parameter doing two things. That makes it hard to name. For I agree with you that the current naming is not optimal. I just haven't heard a good alternative yet. And this has nothing to do with a 'double standard' or anything. It's a big project, we can't fix everything at the same time. Feel free to send a PR to update other parts of the API to be more in line with our naming standards. |
Funny thing, I agree with you here! I hate the fact that it's one parameter doing two things! But having it in one function was pretty common, so even though there's
I think that would be a little awkward. In general, I'd prefer separate functions if it's doing something different. Here I see it as a grey area since it's close but not identical.
In that case, I really don't understand the aversion to single character names. Clarity is what's most important and what's clear is a function of context, prior experiences, and expectations. In general, I agree they're a bad idea but it's situational. Here it comes down to using something that makes sense and what people are likely to expect. I honestly don't think the other single variable names are inappropriate because they make sense in context and longer names would, if anything, hurt clarity. As for |
@magarick While I appreciate your thoughts, I‘m a bit surprised by your - sometimes - harsh language. Meanwhile, the discussion is in the new issue, this one is closed. |
These were recently renamed to
q
, but that does not match our naming policy. Fortunately, it's a simple fix.