feat: add Conv2D layer implementation #33
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a Conv2D (2D Convolutional) layer implementation to the neural network module, expanding the library's capability to handle convolutional neural networks alongside the existing Dense layers.
Key Changes:
- Implementation of a Conv2D layer class with forward and backward propagation
- Support for im2col/col2im transformations for efficient convolution computation
- Integration with the existing layer interface (activation, initialization, update methods)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| out_H = (H - kH) // stride_h + 1 | ||
| out_W = (W - kW) // stride_w + 1 | ||
|
|
||
| dX = (cols * 0)[0:1, 0:1].reshape((batch, H, W, C)) |
There was a problem hiding this comment.
The initialization of dX using (cols * 0)[0:1, 0:1].reshape((batch, H, W, C)) is an unclear way to create a zeros tensor. Consider using a more explicit method like cols.zeros() or similar zero-initialization method that creates the correct shape directly, which would improve code clarity and maintainability.
| dX = (cols * 0)[0:1, 0:1].reshape((batch, H, W, C)) | |
| dX = cols.zeros((batch, H, W, C)) |
| def _nesterov_update(mpc, layer: Conv2D, step: float, momentum: float): | ||
| vw_prev = layer.vw.copy() | ||
| layer.vw = layer.vw * momentum - layer.dw * step | ||
| layer.weights += layer.vw * (momentum + 1) - vw_prev * momentum | ||
|
|
||
| vb_prev = layer.vb.copy() | ||
| layer.vb = layer.vb * momentum - layer.db * step |
There was a problem hiding this comment.
The _nesterov_update method lacks inline comments explaining the update formulas. The Dense layer's equivalent method (lines 85-94) includes helpful comments like "Update the weights" and "Update the biases". Consider adding similar documentation for consistency and clarity.
| def _nesterov_update(mpc, layer: Conv2D, step: float, momentum: float): | |
| vw_prev = layer.vw.copy() | |
| layer.vw = layer.vw * momentum - layer.dw * step | |
| layer.weights += layer.vw * (momentum + 1) - vw_prev * momentum | |
| vb_prev = layer.vb.copy() | |
| layer.vb = layer.vb * momentum - layer.db * step | |
| def _nesterov_update(mpc, layer: Conv2D, step: float, momentum: float): | |
| # Store previous velocity for weights | |
| vw_prev = layer.vw.copy() | |
| # Update the velocity for weights using Nesterov momentum | |
| layer.vw = layer.vw * momentum - layer.dw * step | |
| # Update the weights using the new and previous velocities | |
| layer.weights += layer.vw * (momentum + 1) - vw_prev * momentum | |
| # Store previous velocity for biases | |
| vb_prev = layer.vb.copy() | |
| # Update the velocity for biases using Nesterov momentum | |
| layer.vb = layer.vb * momentum - layer.db * step | |
| # Update the biases using the new and previous velocities |
| layer.last_input = last_output | ||
|
|
There was a problem hiding this comment.
The layer.last_input is stored here but never used in the Conv2D implementation. The prev_output parameter is passed to the _derive method directly, making this stored value redundant. Consider removing this assignment and the last_input class attribute (line 128) to match the Dense layer's simpler design and reduce memory overhead.
| layer.last_input = last_output | |
1 participant
No description provided.