Skip to content
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

Adding support for Neg #119

Merged
merged 2 commits into from
Feb 11, 2025
Merged

Adding support for Neg #119

merged 2 commits into from
Feb 11, 2025

Conversation

dstarkenburg
Copy link
Contributor

@dstarkenburg dstarkenburg commented Feb 11, 2025

Adding support for the Neg layer of ONNX.

PR Checklist

  • Tests are added
  • Documentation, if applicable

src/ops.jl Outdated
@@ -78,6 +78,7 @@ end

add(xs...) = .+(xs...)
sub(xs...) = .-(xs...)
neg(x) = -1 .* x
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just -x (or .-x to be consistent with the code above)?

If you are worried about intersection with sub(xs...), me can modify all 3 functions above like this:

add(x, xs...) = .+(x, xs...)
sub(x, xs...) = .-(x, xs...)
neg(x) = -x

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the way we save the function on the tape be ok with both sub and neg having the same operator, i.e .-(x) .-(xs...)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ONNX, we have Sub(A, B) and Neg(A) - two functions with different names and different number of arguments.

In Julia, we have A - B and -A - two different methods with the same name, but still easily distinguishable by the number of arguments.

There are a few ambiguous corner cases, but in normal flow - when you use - in Julia and Subs/Neg in ONNX - these operations have perfect 1-to-1 mapping. So I don't see a reason to worry.

@dfdx dfdx merged commit 3f30c4e into FluxML:master Feb 11, 2025
2 checks passed
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