Skip to content

Add negative and positive element-wise specs #55

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

Merged
merged 8 commits into from
Oct 22, 2020
Merged

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Oct 19, 2020

This PR

  • adds specifications for the element-wise APIs: negative and positive.
  • follows the conventions set forth by other element-wise APIs.

Notes

  • While negative is relatively common across array libraries, positive is less common. The main impetus for this PR is to ensure functional API equivalents to the magic methods __neg__ and __pos__, respectively, as discussed, and endorsed, in consortium meetings.
  • Torch uses the name neg, while NumPy et al and TensorFlow use negative. I've followed NumPy precedent here, rather than magic method conventions.

@rgommers
Copy link
Member

Do we really need positive? I can't remember an explicit discussion on that function. Given that it's basically a do-nothing call, I'm not sure why anyone would want to use it. NumPy does have it I see, never noticed it before. bool input raises an error there, which makes sense - but that isn't specified in this PR.

@kgryte
Copy link
Contributor Author

kgryte commented Oct 22, 2020

I suppose it depends if we believe that the function should be included for completeness. This is true both for __pos__ equivalence (if __pos__ should be defined to be with) and because negative is defined.

Thanks for pointing out the NumPy exception (and deprecation for __pos__ and non-numerical arrays). At minimum, I'll update this PR to indicate that both negative and positive should only support numerical arrays and not bool.

@rgommers
Copy link
Member

I suppose it depends if we believe that the function should be included for completeness.

Yeah it does make sense from that perspective. If it's the one dunder method that would otherwise be missing, let's include it.

@rgommers
Copy link
Member

Discussed in today's call, everyone seemed happy to also include positive. Merging.

@rgommers rgommers merged commit 424fb71 into master Oct 22, 2020
@rgommers rgommers deleted the elementwise-1 branch October 22, 2020 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants