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

fix axis use for sum #543

Merged
merged 3 commits into from
Jan 22, 2025
Merged

fix axis use for sum #543

merged 3 commits into from
Jan 22, 2025

Conversation

timmysilv
Copy link
Collaborator

@timmysilv timmysilv commented Jan 20, 2025

User description

Note: This is not critically needed, because users can just pass axis=[x] instead of axis=x and it will work - it's more aesthetic than anything.

Context:
np.sum only accepts ints for the axis, or tuples of ints. We recently updated the backend manager to match numpy by renaming axes to axis, but unexpectedly do not accept a single int as the singular name implies. Even stranger, np.sum(..., axis=[1, 2]) fails because axis must be a tuple and not a list. We cast to tuple internally so that usage is alright with mrmustard.math.sum.

Description of the Change:
Accept single ints as well as sequences of ints.

Benefits:
mrmustard.math.sum better matches the backend library conventions.

Possible Drawbacks:
N/A


PR Type

Bug fix, Enhancement


Description

  • Updated math.sum to accept single integers for axis.

  • Replaced list-based axis arguments with integers or tuples across the codebase.

  • Improved compatibility with backend libraries by aligning math.sum behavior.

  • Adjusted related tests to reflect the updated axis handling.


Changes walkthrough 📝

Relevant files
Bug fix
6 files
circuit_components.py
Updated `math.sum` calls to use integer `axis`.                   
+1/-1     
base.py
Replaced list-based `axis` with integer in `math.sum`.     
+3/-3     
array_ansatz.py
Fixed `math.sum` usage with integer `axis`.                           
+1/-1     
polyexp_ansatz.py
Replaced list-based `axis` with integer in `math.sum`.     
+3/-3     
gaussian_integrals.py
Updated `math.sum` to use integer `axis`.                               
+1/-1     
representations.py
Fixed `math.sum` usage with integer `axis`.                           
+1/-1     
Enhancement
3 files
backend_manager.py
Enhanced `math.sum` to accept int or sequence for `axis`.
+3/-3     
backend_numpy.py
Updated `math.sum` to support int or sequence for `axis`.
+1/-1     
backend_tensorflow.py
Adjusted `math.sum` to handle int or tuple for `axis`.     
+1/-1     
Tests
1 files
test_cft.py
Adjusted test to reflect updated `math.sum` behavior.       
+1/-1     

Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 95
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Consistency

    The type hint for axis parameter in sum() method uses both Sequence[int] and tuple[int] across different backend implementations. Consider standardizing the type hints for consistency.

    def sum(self, array: Tensor, axis: int | Sequence[int] | None = None):
        r"""The sum of array.
    
        Args:
            array: The array to take the sum of
            axis (int | Sequence[int] | None): The axis/axes to sum over
    
        Returns:
            The sum of array
        """
        if axis is not None and not isinstance(axis, int):
            neg = [a for a in axis if a < 0]
            pos = [a for a in axis if a >= 0]
            axis = tuple(sorted(neg) + sorted(pos)[::-1])
        return self._apply("sum", (array, axis))

    Copy link
    Contributor

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @timmysilv timmysilv added the no changelog Pull request does not require a CHANGELOG entry label Jan 20, 2025
    Copy link

    codecov bot commented Jan 20, 2025

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 90.01%. Comparing base (2bd6489) to head (b5bbba6).
    Report is 1 commits behind head on develop.

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #543      +/-   ##
    ===========================================
    - Coverage    90.05%   90.01%   -0.04%     
    ===========================================
      Files          102      102              
      Lines         6222     6222              
    ===========================================
    - Hits          5603     5601       -2     
    - Misses         619      621       +2     
    Files with missing lines Coverage Δ
    mrmustard/lab_dev/circuit_components.py 98.44% <ø> (ø)
    mrmustard/lab_dev/states/base.py 96.66% <100.00%> (ø)
    mrmustard/math/backend_manager.py 98.17% <100.00%> (ø)
    mrmustard/math/backend_numpy.py 100.00% <ø> (ø)
    mrmustard/math/backend_tensorflow.py 100.00% <ø> (ø)
    mrmustard/physics/ansatz/array_ansatz.py 100.00% <100.00%> (ø)
    mrmustard/physics/ansatz/polyexp_ansatz.py 98.57% <100.00%> (ø)
    mrmustard/physics/gaussian_integrals.py 99.39% <ø> (ø)
    mrmustard/physics/representations.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 2bd6489...b5bbba6. Read the comment docs.

    Copy link
    Collaborator

    @ziofil ziofil left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    thanks!

    @timmysilv timmysilv merged commit 2de68b2 into develop Jan 22, 2025
    10 checks passed
    @timmysilv timmysilv deleted the sum-axis-fix branch January 22, 2025 16:23
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix enhancement New feature or request no changelog Pull request does not require a CHANGELOG entry Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants