You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
gh-187 says clip is universally implemented by libraries. This looks to me like a useful function to add. IIRC we had some issues with clip behavior in numpy, we should dig those up to check for possible semantics issues.
No name is ideal - min/max are nicer names, but not everyone may be happy that it conflicts with builtin names. a_min and a_max are bad though, given that we consistently name our input arrays x and not a. So how about:
defclip(x, min, max, /): # all positional-only args to avoid incompatibilities
No name is ideal - min/max are nicer names, but not everyone may be happy that it conflicts with builtin names. a_min and a_max are bad though, given that we consistently name our input arrays x and not a. So how about:
defclip(x, min, max, /): # all positional-only args to avoid incompatibilities
I would lean towardsclip(x, /, min=None, max=None). The naming conflict is only inside the array library, not in user code, which can call these variables whatever it likes.
Side note: this is yet another example (like unstack and moveaxis) of functionality that could perhaps have a universal default implementation terms of other array API functionality (specifically where).
The naming conflict is only inside the array library, not in user code, which can call these variables whatever it likes.
Only if they're positional, right?
I actually do like your signature better - clip(x, max=3) is nicer than having to write clip(x, None, 3). It's just another thing then that we have to deal with when we aim to converge NumPy's main namespace to the array API standard. But I guess it's not too bad, there's already **kwargs in np.clip, so using aliases is easy.
The naming conflict is only inside the array library, not in user code, which can call these variables whatever it likes.
Only if they're positional, right?
I only get lint errors for overriding builtins if I define variables with the same name as a builtin. Something like clip(x, max=3) is fine -- the lint error is defining a variable named max, not for using a keyword argument.
clip(x, max=max) would be problematic, but there's nothing in the interface that requires calling the variable max. Users can write something like clip(x, max=a_max) if desired.
I agree that clip is clear enough that it seems fine to allow passing positional and min, max seems OK (no strong opinions on it though).
I would lean against a_min, etc. even if that is what NumPy has. The alternative I may seriously consider is lower and upper (I have not quite made up my mind whether that is the better or worse name for bounds – mathematically speaking).
I don't want to volunteer it, but it shouldn't be hard to change that in NumPy or fix in a compat namespace. And yes, it will mean that it isn't the same as a chained minimum/maximum call.
Activity
rgommers commentedon Sep 23, 2022
gh-187 says
clip
is universally implemented by libraries. This looks to me like a useful function to add. IIRC we had some issues withclip
behavior innumpy
, we should dig those up to check for possible semantics issues.rgommers commentedon Oct 5, 2022
This wasn't too bad actually, a lot of it was segfaults and whether it was a ufunc or not. The relevant ones:
And of course we need to choose keyword names, always a hard problem. Existing signatures don't match:
np.clip(a, a_min, a_max, out=None, **kwargs)
torch.clip(input, min=None, max=None, *, out=None)
No name is ideal -
min
/max
are nicer names, but not everyone may be happy that it conflicts with builtin names.a_min
anda_max
are bad though, given that we consistently name our input arraysx
and nota
. So how about:shoyer commentedon Oct 5, 2022
I would lean towards
clip(x, /, min=None, max=None)
. The naming conflict is only inside the array library, not in user code, which can call these variables whatever it likes.Side note: this is yet another example (like
unstack
andmoveaxis
) of functionality that could perhaps have a universal default implementation terms of other array API functionality (specificallywhere
).rgommers commentedon Oct 5, 2022
Only if they're positional, right?
I actually do like your signature better -
clip(x, max=3)
is nicer than having to writeclip(x, None, 3)
. It's just another thing then that we have to deal with when we aim to converge NumPy's main namespace to the array API standard. But I guess it's not too bad, there's already**kwargs
innp.clip
, so using aliases is easy.shoyer commentedon Oct 5, 2022
I only get lint errors for overriding builtins if I define variables with the same name as a builtin. Something like
clip(x, max=3)
is fine -- the lint error is defining a variable namedmax
, not for using a keyword argument.clip(x, max=max)
would be problematic, but there's nothing in the interface that requires calling the variablemax
. Users can write something likeclip(x, max=a_max)
if desired.jakirkham commentedon Oct 5, 2022
cc @seberg (in case you have thoughts here)
seberg commentedon Oct 6, 2022
I agree that clip is clear enough that it seems fine to allow passing positional and
min
,max
seems OK (no strong opinions on it though).I would lean against
a_min
, etc. even if that is what NumPy has. The alternative I may seriously consider islower
andupper
(I have not quite made up my mind whether that is the better or worse name for bounds – mathematically speaking).rgommers commentedon Oct 7, 2022
Questions that were brought up yesterday:
min
andmax
inputs participate in determining the output dtype, or should the output dtype equal the input dtype?== input_dtype
min
andmax
allowed to be arrays, or only scalars?oleksandr-pavlyk commentedon Oct 25, 2023
if
min
andmax
do not participate in the type promotion, this makesclip
not equivalent toxp.maximum(xp.minimum(x, min), max)
in functional behavior.In NumPy both
clip
'smin
andmax
arguments participate in the type promotion:seberg commentedon Oct 25, 2023
I don't want to volunteer it, but it shouldn't be hard to change that in NumPy or fix in a compat namespace. And yes, it will mean that it isn't the same as a chained minimum/maximum call.
3 remaining items