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

Trying to add ConvTranspose #95

Closed
ordicker opened this issue Jul 18, 2023 · 1 comment
Closed

Trying to add ConvTranspose #95

ordicker opened this issue Jul 18, 2023 · 1 comment

Comments

@ordicker
Copy link
Contributor

Motivation and description

Hi,

I'm trying to add a new ops called ConvTranspose.
And I have stumbled upon flipweights and the whole from_nnlib_*`from_onnx_*` conversions.

So I have two questions,

  1. Is flipweight is the same as nnlib.conv flipped keyword? If so, should we use it instead?
  2. Can we have a better/simpler args conversion?
    When one wants to implement new op, he should add:
  • save_node!(g::Graph,::OpConfig{:ONNX,<new_op>) on save.jl
  • load_node!(tape::Tape, OpConfig{:ONNX, <new_op>} on load.jl
  • if the op has parameters, then implement the conversion on conversions.jl
  • implement the function itself on ops.jl
    On my opinion, that is a bit all over the place.
    Could we make it simpler? Or maybe with the same complexity but have a file per op. So for example, the whole Conv related stuff would be on conv.jl. (that way we could have a template for adding new op).

Possible Implementation

What do you think?

@dfdx
Copy link
Collaborator

dfdx commented Jul 19, 2023

  1. I don't remember any specific reason why I used flipweights(w) instead of flipped keyword argument, so it indeed may be a better solution.
  2. To add a new operation, one must only add save_node!() and load_node(). For many operations these are pretty simple, e.g. see relu, tanh or batched_mul. In some cases, Julia doesn't have a direct counterpart for ONNX operations, e.g. in Flatten, Gemm, Conv. In this case, the simplest solution is usually to add a proxy Julia function with semantics similar to the ONNX version, e.g. onnx_flatten or onnx_gather. Similarly, sometimes ONNX attributes don't map well to the keyword arguments of the corresponding Julia functions, e.g. in Conv / NNlib.conv. In this case, a helper function for argument conversions may be helpful. Sometimes several ops share the same conversion utilities, that's why they are in a separate file.

I don't mind refactoring if it simplifies things. But in my experience, trying to optimize the codebase of ONNX.jl often leads to approximately the same level of complexity and a lot of frustration 😄

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

No branches or pull requests

2 participants