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

convolution added to numpy backend #517

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

ziofil
Copy link
Collaborator

@ziofil ziofil commented Nov 1, 2024

User description

In order to compute the PNR conditional matrix we need the convolution function to work in the numpy backend. I added it.


PR Type

enhancement


Description

  • Added a new convolution function to the numpy backend, enabling 2D convolution operations similar to TensorFlow's tf.nn.convolution.
  • The convolution function uses scipy.signal.convolve2d for efficient computation and supports 'SAME' and 'VALID' padding options.
  • Updated the poisson function in backend_manager.py to ensure consistent dtype usage with rate.

Changes walkthrough 📝

Relevant files
Enhancement
backend_manager.py
Update dtype handling in `poisson` function                           

mrmustard/math/backend_manager.py

  • Updated poisson function to use rate.dtype for arange.
+1/-2     
backend_numpy.py
Add 2D convolution function to numpy backend                         

mrmustard/math/backend_numpy.py

  • Added convolution function for 2D convolution using numpy.
  • Utilized scipy.signal.convolve2d for efficient 2D convolution.
  • Implemented padding logic for 'SAME' and 'VALID' options.
  • +54/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @ziofil ziofil added the no changelog Pull request does not require a CHANGELOG entry label Nov 1, 2024
    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🏅 Score: 85
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Performance Concern
    The convolution function performs the operation only for the first channel. This might lead to incorrect results for multi-channel inputs.

    Potential Bug
    The convolution function ignores the 'data_format' parameter, which might cause issues if the function is called with different data formats.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Nov 1, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Utilize all input channels in the convolution operation instead of only the first channel

    The current implementation only uses the first channel of the input array for
    convolution. Modify the code to utilize all input channels, which is typically
    expected in a convolution operation.

    mrmustard/math/backend_numpy.py [169-173]

    -array = np.pad(
    -    array[:, :, :, 0], ((0, 0), (pad_h, pad_h), (pad_w, pad_w)), mode="constant"
    -)
    -...
    -array = array[:, :, :, 0]
    +if padding == "SAME":
    +    array = np.pad(
    +        array, ((0, 0), (pad_h, pad_h), (pad_w, pad_w), (0, 0)), mode="constant"
    +    )
    +# No need for the else case as we're not modifying the array for 'VALID' padding
    Suggestion importance[1-10]: 9

    Why: The suggestion corrects a critical oversight by ensuring that all input channels are utilized in the convolution operation, which is a standard expectation. This change is crucial for the correctness and completeness of the convolution process.

    9
    Enhancement
    Implement multi-channel convolution support to handle all output channels

    Consider using broadcasting to handle multiple output channels efficiently, instead
    of only processing the first channel. This can improve performance and functionality
    for multi-channel convolutions.

    mrmustard/math/backend_numpy.py [163-189]

    -# Reshape filter to 2D for convolution
    -filter_2d = filters[:, :, 0, 0]
    -...
    -# Perform convolution for each batch
    +# Perform convolution for each batch and channel
     for b in range(batch):
    -    # Convolve using scipy's convolve2d which is more efficient than np.convolve for 2D
    -    output[b, :, :, 0] = scipy_convolve2d(
    -        array[b],
    -        np.flip(np.flip(filter_2d, 0), 1),  # Flip kernel for proper convolution
    -        mode="valid",
    -    )
    +    for c in range(out_channels):
    +        output[b, :, :, c] = scipy_convolve2d(
    +            array[b],
    +            np.flip(np.flip(filters[:, :, 0, c], 0), 1),
    +            mode="valid",
    +        )
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a significant limitation in the current implementation by enabling multi-channel convolution, which is essential for handling all output channels efficiently. This enhancement improves both functionality and performance for multi-channel data.

    8
    Possible issue
    Improve padding calculation to correctly handle both odd and even kernel sizes

    The padding calculation for 'SAME' mode assumes odd kernel sizes. For even kernel
    sizes, this might lead to asymmetric padding. Consider adjusting the padding
    calculation to handle both odd and even kernel sizes correctly.

    mrmustard/math/backend_numpy.py [166-171]

     # For SAME padding, calculate padding sizes
     if padding == "SAME":
    -    pad_h = (kernel_h - 1) // 2
    -    pad_w = (kernel_w - 1) // 2
    +    pad_h = max(kernel_h - 1, 0)
    +    pad_w = max(kernel_w - 1, 0)
    +    pad_top = pad_h // 2
    +    pad_bottom = pad_h - pad_top
    +    pad_left = pad_w // 2
    +    pad_right = pad_w - pad_left
         array = np.pad(
    -        array[:, :, :, 0], ((0, 0), (pad_h, pad_h), (pad_w, pad_w)), mode="constant"
    +        array[:, :, :, 0], ((0, 0), (pad_top, pad_bottom), (pad_left, pad_right)), mode="constant"
         )
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion corrects the padding calculation for 'SAME' mode to handle both odd and even kernel sizes, preventing potential asymmetric padding issues. This change enhances the robustness and correctness of the convolution operation.

    7
    Best practice
    Add input validation to ensure correct tensor shapes and dimensions

    The function assumes the input array has exactly 4 dimensions. Consider adding input
    validation to ensure the input shape is correct and provide meaningful error
    messages if not.

    mrmustard/math/backend_numpy.py [159-160]

    +# Validate input shapes
    +if array.ndim != 4 or filters.ndim != 4:
    +    raise ValueError(f"Expected 4D tensors, got array with {array.ndim}D and filters with {filters.ndim}D")
    +
     # Extract shapes
     batch, in_height, in_width, in_channels = array.shape
    -kernel_h, kernel_w, _, out_channels = filters.shape
    +kernel_h, kernel_w, filter_in_channels, out_channels = filters.shape
     
    +# Validate channel dimensions
    +if in_channels != filter_in_channels:
    +    raise ValueError(f"Input channels ({in_channels}) must match filter input channels ({filter_in_channels})")
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding input validation ensures that the function receives tensors with the expected dimensions, providing meaningful error messages if not. This improves the reliability and user-friendliness of the function.

    6

    💡 Need additional feedback ? start a PR chat

    Copy link

    codecov bot commented Nov 1, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 89.80%. Comparing base (3a1d8bb) to head (b264136).

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #517      +/-   ##
    ===========================================
    + Coverage    89.77%   89.80%   +0.03%     
    ===========================================
      Files           92       92              
      Lines         6072     6072              
    ===========================================
    + Hits          5451     5453       +2     
    + Misses         621      619       -2     
    Files with missing lines Coverage Δ
    mrmustard/math/backend_manager.py 98.16% <100.00%> (-0.01%) ⬇️
    mrmustard/math/backend_numpy.py 100.00% <100.00%> (ø)

    ... and 1 file with indirect coverage changes


    Continue to review full report in Codecov by Sentry.

    Legend - Click here to learn more
    Δ = absolute <relative> (impact), ø = not affected, ? = missing data
    Powered by Codecov. Last update 3a1d8bb...b264136. Read the comment docs.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    enhancement New feature or request no changelog Pull request does not require a CHANGELOG entry Review effort [1-5]: 3
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant