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

ops::split() shorthand function is broken #357

Open
Corallus-Caninus opened this issue Apr 15, 2022 · 4 comments
Open

ops::split() shorthand function is broken #357

Corallus-Caninus opened this issue Apr 15, 2022 · 4 comments
Labels

Comments

@Corallus-Caninus
Copy link
Contributor

Corallus-Caninus commented Apr 15, 2022

I cannot construct a split operation. when given the appropriate types in ops::split() the attribute num_split does not get assigned.
I'd reference the line but the file is too large for github, in my IDE it is 111704 in src/ops/ops_impl

there seems to be a discrepancy between num_split and split_dim as well.

It also doesnt seem like a great idea to have if statements for build_impl codegen where those attributes are mandatory without a default value for the node but I am still learning the codebase.

If anyone sees this please let me know how I can help otherwise I'll wing it.

@Corallus-Caninus
Copy link
Contributor Author

I was able to work around this by building the operation without the helper function split() by doing the following:

let split_outputs = ops::Split::new()
    .num_split(num_splits)
    .build(axis_zero, input, scope)?;

where axis_zero is ops::constant(0,scope);
and num_splits is ops::constant as well

however this doesnt close this issue since the helper function is broken and in the best case misleading.

@Corallus-Caninus Corallus-Caninus changed the title Split operation is broken Split operation's shorthand function is broken Apr 17, 2022
@Corallus-Caninus Corallus-Caninus changed the title Split operation's shorthand function is broken ops::split() shorthand function is broken Apr 17, 2022
@dskkato
Copy link
Contributor

dskkato commented Apr 18, 2022

The gerated code is not broken, but default values are still missing as you pointed out. We may need a help to fix it.

Edited : Default value is not irrelevant.

@dskkato
Copy link
Contributor

dskkato commented Apr 18, 2022

FYI
ops code was generated from this definition file.
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/ops/ops.pbtxt

And the code was generated by tensorflow-op-codegen crate in this repository.

@adamcrume adamcrume added the bug label Apr 20, 2022
@dskkato
Copy link
Contributor

dskkato commented Apr 20, 2022

Here is the Split definition in the pbtxt.

op {
  name: "Split"
  input_arg {
    name: "split_dim"
    type: DT_INT32
  }
  input_arg {
    name: "value"
    type_attr: "T"
  }
  output_arg {
    name: "output"
    type_attr: "T"
    number_attr: "num_split"
  }
  attr {
    name: "num_split"
    type: "int"
    has_minimum: true
    minimum: 1
  }
  attr {
    name: "T"
    type: "type"
  }
}

In this case, it seems we should not discard has_minimum and minimum field. Another cases would be has_maximum/maximum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants