Transp2d + Transp1d refactor + minor syntax updates #36
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for Conv2DTranspose (2D transposed convolution/deconvolution) layers and refactors the existing Conv1DTranspose implementation with improved variable naming. The changes also include extensive string formatting updates across multiple Python files for consistency, converting single quotes to double quotes and improving code readability through better string concatenation patterns.
Key Changes:
- Implementation of Conv2DTranspose layer with corresponding C function, Python layer writer, and test coverage
- Refactored Conv1DTranspose C implementation with clearer variable names (e.g.,
cs→output_start_idx) - Standardized string formatting throughout
weights2c.pyandlayer2c.pyusing double quotes and improved concatenation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_convtranspose_layers.py | Added Conv2DTranspose test cases and refactored Conv1DTranspose test with clearer variable names matching C implementation; reorganized imports |
| keras2c/weights2c.py | Added _write_weights_Conv2DTranspose method with kernel transpose logic; extensive string formatting updates to use double quotes and improved concatenation throughout; refactored class definition to remove parentheses |
| keras2c/layer2c.py | Added _write_layer_Conv2DTranspose method for generating C layer calls; extensive string formatting updates matching weights2c.py style; reorganized imports |
| include/k2c_include.h | Added function declaration for k2c_conv2d_transpose |
| include/k2c_conv_transpose_layer.c | Refactored Conv1DTranspose with improved variable naming; implemented Conv2DTranspose function with 2D spatial convolution logic including bounds checking and cropping |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Pre-calculate dimensional steps for Kernel | ||
| // Kernel index math: z0 * (cols*in*out) + z1 * (in*out) + q * (out) + k | ||
| // Note: This matches the "Out-Channel Last" memory layout of the reference. |
There was a problem hiding this comment.
The kernel indexing comment on line 126 mentions "Out-Channel Last" memory layout, but this is inconsistent with the earlier comment on lines 799-801 in weights2c.py which states the C code expects (rows, cols, in_channels, out_channels). "Out-Channel Last" would typically mean the out_channels dimension is the last one, which matches the stated expectation. However, the phrase "Out-Channel Last" is not a standard term and could be confusing. Consider using clearer terminology like "output channels in the last dimension" or referencing the exact shape expected.
| // Note: This matches the "Out-Channel Last" memory layout of the reference. | |
| // Note: This matches a kernel layout of (rows, cols, in_channels, out_channels), | |
| // i.e., with output channels in the last dimension. |
| if cfg["use_bias"]: | ||
| bias = weights[1] | ||
| else: | ||
| bias = np.zeros(kernel.shape[3]) |
There was a problem hiding this comment.
After the kernel transpose on line 804, the kernel shape changes from (rows, cols, out_channels, in_channels) to (rows, cols, in_channels, out_channels). However, line 808 initializes the bias with 'kernel.shape[3]' which now refers to out_channels after the transpose. This is correct since bias should have one value per output channel (filter). However, for code clarity, it would be better to use the variable 'n_filters' from cfg or calculate it before the transpose, or add a comment explaining that shape[3] is out_channels after the transpose.
| stride_h = np.random.randint(1, max(k_rows, 2)) | ||
| stride_w = np.random.randint(1, max(k_cols, 2)) |
There was a problem hiding this comment.
The variable names in the Conv2DTranspose test are inconsistent with the variable names used in Conv1DTranspose. Conv1DTranspose uses 'stride' (singular) for stride parameter on line 34 and 46, while Conv2DTranspose uses 'stride_h' and 'stride_w' on lines 67-68 but then passes them as a tuple with parameter name 'strides' (plural) on line 80. For consistency and to match the underlying C function parameters which expect arrays, consider using consistent naming like 'strides' or keeping both individual variables but using them consistently.
| # Keras stores kernel as (rows, cols, out_channels, in_channels) | ||
| # C code expects (rows, cols, in_channels, out_channels) | ||
| # So we need to transpose the last two dimensions |
There was a problem hiding this comment.
The kernel transpose operation on line 804 transposes the last two dimensions (from shape (rows, cols, out_channels, in_channels) to (rows, cols, in_channels, out_channels)). However, the comment on lines 799-801 states that Keras stores kernel as (rows, cols, out_channels, in_channels) and C expects (rows, cols, in_channels, out_channels). This needs verification - according to Keras documentation for Conv2DTranspose, the kernel shape is actually (rows, cols, filters, input_channels) where filters are the output channels. The transpose appears correct, but it would be clearer if the comment explicitly mentioned that 'filters' means 'out_channels' and clarified the dimension indices being transposed.
| # Keras stores kernel as (rows, cols, out_channels, in_channels) | |
| # C code expects (rows, cols, in_channels, out_channels) | |
| # So we need to transpose the last two dimensions | |
| # For Conv2DTranspose, Keras stores the kernel as | |
| # (rows, cols, filters, input_channels), where "filters" are the | |
| # output channels (out_channels). The C code expects | |
| # (rows, cols, in_channels, out_channels), so we transpose the last | |
| # two dimensions (swap axis 2 and 3). |
| size_t output_start_idx = 0; // cs | ||
| size_t output_end_idx = 0; // ce | ||
| size_t output_raw_idx = 0; // ts | ||
| size_t kernel_offset = 0; // ks |
There was a problem hiding this comment.
In the comment on lines 31-35, the variable names are being renamed for clarity (e.g., 'cs' to 'output_start_idx'). However, these comments show the old variable names in a format that suggests they are being defined (e.g., "// cs"). This notation is somewhat ambiguous. Consider clarifying this with a format like "// was: cs" or "// renamed from: cs" to make it clear these are the old names being replaced.
| size_t output_start_idx = 0; // cs | |
| size_t output_end_idx = 0; // ce | |
| size_t output_raw_idx = 0; // ts | |
| size_t kernel_offset = 0; // ks | |
| size_t output_start_idx = 0; // was: cs | |
| size_t output_end_idx = 0; // was: ce | |
| size_t output_raw_idx = 0; // was: ts | |
| size_t kernel_offset = 0; // was: ks |
| const size_t *stride, const size_t *dilation, | ||
| const size_t *padding, k2c_activationType *activation) |
There was a problem hiding this comment.
The dilation parameter is passed to the function but never used in the implementation. While weights2c.py validates that dilation must be 1 (lines 749-750), the C function still accepts the parameter and the comment on line 93 notes this limitation. For better code clarity and maintainability, consider either: (1) removing the dilation parameter entirely if it's always 1, or (2) implementing proper dilation support, or (3) adding a runtime assertion in the C code to validate dilation values, similar to the Python validation. The current approach where the parameter is passed but ignored is confusing and could lead to bugs if the Python validation is bypassed or removed in the future.
Added convtranspose2d, and changed ambiguous naming structure in convtranspose1d. Also changed some string syntax to be more consistent for keras2c main function.
Note
Adds full 2D transposed convolution support and minor 1D refactor.
k2c_conv2d_transposeimplementation ininclude/k2c_conv_transpose_layer.cwith channels-last layout, bias add, and activation; declared ink2c_include.hLayers2C._write_layer_Conv2DTransposeemits calls tok2c_conv2d_transpose; existing Conv1DTranspose emission cleaned upWeights2C._write_weights_Conv2DTransposewrites strides/dilation/padding (crop), transposes kernel to{rows, cols, in_ch, out_ch}, and emits biask2c_conv1d_transposetests/test_convtranspose_layers.pycovering random-configConv1DTransposeandConv2DTransposeWritten by Cursor Bugbot for commit 93f0bed. This will update automatically on new commits. Configure here.