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

Quantity initialization does not call jax.numpy.asarray when mantissa is a python number #94

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

chaoming0625
Copy link
Member

@chaoming0625 chaoming0625 commented Jan 14, 2025

This pull request includes several changes to improve the handling of numerical operations and enhance the accuracy of unit tests in the brainunit package. The most important changes involve modifying the handling of numerical types, updating test assertions for better precision, and importing necessary libraries.

Numerical operations improvements:

  • brainunit/_base.py: Updated the __init__, tolist, and get methods to handle numerical types more accurately and avoid unnecessary conversions. [1] [2] [3]

Test precision enhancements:

  • brainunit/_base_test.py: Modified assertions in test_unit_with_factor and test_display methods to use u.math.isclose for better precision in floating-point comparisons. [1] [2]
  • brainunit/_celsius_test.py: Updated assertions in test1 and test2 methods to use u.math.allclose and np.isclose for improved accuracy.
  • brainunit/constants_test.py: Enhanced the test_quantity_constants_and_unit_constants method to use u.math.isclose for precise comparison of constants.

Library imports:

Summary by Sourcery

Fix quantity initialization for Python numbers and improve test assertions.

Bug Fixes:

  • Fix issue where Quantity initialization did not correctly handle Python numbers.
  • Improve the accuracy of floating-point comparisons in tests by using u.math.isclose and np.isclose.

Copy link
Contributor

sourcery-ai bot commented Jan 14, 2025

Reviewer's Guide by Sourcery

This PR addresses a bug in the Quantity initialization where jax.numpy.asarray was not called when the mantissa was a Python number. This could lead to incorrect type handling in downstream operations. The fix involves always converting the mantissa to a JAX array. Additionally, several unit tests were updated to use more precise comparison methods like u.math.isclose and np.isclose for floating-point values, improving the accuracy and reliability of the tests.

Sequence diagram for Quantity initialization with Python number

sequenceDiagram
    participant Client
    participant Quantity
    participant mantissa

    Client->>Quantity: __init__(mantissa=python_number)
    Note over Quantity: Check if mantissa is a number
    alt mantissa is a number
        Quantity->>mantissa: Keep original value
    else mantissa is other type
        Quantity->>mantissa: Convert to array
    end
    Quantity-->>Client: Return Quantity instance
Loading

Class diagram for Quantity class changes

classDiagram
    class Quantity {
        +mantissa
        +unit
        +__init__(mantissa, unit)
        +tolist()
        +get(indices_are_sorted, unique_indices, fill_value)
    }
    note for Quantity "Modified mantissa handling:
    - Skip jnp.array for numbers
    - Updated tolist() for number types
    - Modified get() fill_value handling"
Loading

Flow diagram for mantissa type handling

flowchart TD
    A[Input mantissa] --> B{Is Python/JAX number?}
    B -->|Yes| C[Keep original value]
    B -->|No| D[Convert to array]
    C --> E[Create Quantity]
    D --> E
    E --> F[Use in operations]
    F --> G{Need conversion?}
    G -->|Yes| H[tolist/get methods]
    G -->|No| I[Direct use]
Loading

File-Level Changes

Change Details Files
Fix Quantity initialization to handle Python numbers correctly.
  • Removed unnecessary conditional check and ensured that the mantissa is always converted to a JAX array using jnp.array.
  • Corrected the logic in tolist and get methods to handle numerical types accurately and avoid unnecessary conversions when dealing with Python numbers as mantissas
brainunit/_base.py
Update unit tests to use more precise floating-point comparisons.
  • Replaced self.assertTrue with u.math.isclose for floating-point comparisons in brainunit/_base_test.py and brainunit/constants_test.py.
  • Used u.math.allclose and np.isclose for improved accuracy in brainunit/_celsius_test.py.
brainunit/_base_test.py
brainunit/_celsius_test.py
brainunit/constants_test.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @chaoming0625 - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 81 to +87
for c in constants_list:
q_c = getattr(quantity_constants, c)
u_c = getattr(unit_constants, c)
assert q_c.to_decimal(q_c.unit) == (1. * u_c).to_decimal(
q_c.unit), f"Mismatch between {c} in quantity_constants and unit_constants"
assert u.math.isclose(
q_c.to_decimal(q_c.unit),
(1. * u_c).to_decimal(q_c.unit)
), f"Mismatch between {c} in quantity_constants and unit_constants"
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (code-quality): Avoid loops in tests. (no-loop-in-tests)

ExplanationAvoid complex code, like loops, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:

  • loops
  • conditionals

Some ways to fix this:

  • Use parametrized tests to get rid of the loop.
  • Move the complex logic into helpers.
  • Move the complex part into pytest fixtures.

Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / Don't Put Logic in Tests

@chaoming0625 chaoming0625 merged commit b5cc99c into main Jan 14, 2025
29 of 30 checks passed
@chaoming0625 chaoming0625 deleted the update branch January 14, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant