pad-input-fix: adding support for pads as attributes #2195
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2195 +/- ##
==========================================
- Coverage 86.08% 86.08% -0.01%
==========================================
Files 695 695
Lines 88995 89036 +41
==========================================
+ Hits 76612 76644 +32
- Misses 12383 12392 +9 ☔ View full report in Codecov by Sentry. |
| if node.inputs.len() < 2 { | ||
| panic!("Pad: must provide at least two inputs") | ||
| if node.inputs.is_empty() { | ||
| panic!("Pad: must be provide data as input") |
There was a problem hiding this comment.
Weird formulation. Makes more sense when you remove the "be": "Pad: must provide data as input"
| @@ -768,9 +768,20 @@ pub fn tile_config(node: &Node) -> TileConfig { | |||
|
|
|||
| /// Create a PadConfig from the attributes of the node | |||
| pub fn pad_config(node: &Node) -> PadConfig { | |||
| fn get_input_values(node: &Node, index: usize) -> Vec<i64> { | |||
There was a problem hiding this comment.
Since this is only used to retrieve the pads input values, I would rename to get_pads_input (and the index can be hardcoded).
| "pads" => { | ||
| pads = value | ||
| .clone() | ||
| .into_i64s() | ||
| .iter() | ||
| .map(|&x| { | ||
| if x < 0 { | ||
| panic!("Pad: Negative pad is not supported"); | ||
| } | ||
| x as usize | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
If we are supporting the pads as attribute, which corresponds to version 2 of the operator, then we should also parse the value attribute to make sure we are correctly capturing all parameters of the node.
Make sure it doesn't come in conflict with the constant_value input parsing for later versions of the operator 🙂
|
@mepatrick73, was this supposed to fix #2151? I am still seeing the same issue... |
Pull Request Template
Checklist
run-checks allscript has been executed.Related Issues/PRs
#2151
Changes
Added support to pad being passed as an attribute. Also adding a check for the mode
Testing
Ran user model mentionned in #2151