Skip to content

Refactor division methods in Unit class and improve reverse functionality#38

Merged
chaoming0625 merged 1 commit intomainfrom
fix
Jun 2, 2025
Merged

Refactor division methods in Unit class and improve reverse functionality#38
chaoming0625 merged 1 commit intomainfrom
fix

Conversation

@chaoming0625
Copy link
Member

@chaoming0625 chaoming0625 commented Jun 2, 2025

Now, 1/unit is a Quantity, rather a Unit.

Summary by Sourcery

Extract reciprocal logic into a new Unit.reverse() method, refactor the reverse-division operator to use it, and update tests accordingly.

New Features:

  • Introduce a public Unit.reverse() method to return the reciprocal unit.

Enhancements:

  • Refactor Unit.__rdiv__ to delegate to Unit.__div__ or Quantity and use reverse() instead of inline reciprocal logic.
  • Simplify reverse-division by replacing 1/self unit construction with self.reverse().

Tests:

  • Update tests to use reverse() for asserting reciprocal units instead of 1 / unit or 1 / quantity.
  • Comment out deprecated tests that directly asserted isinstance(1 / unit, Unit).
  • Remove redundant complex unit string-representation test cases.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jun 2, 2025

Reviewer's Guide

This PR refactors the Unit class’s division logic by extracting inversion into a new reverse() method, simplifies rdiv to use reverse(), and updates/removes tests to reference reverse() instead of computing 1/u and drop outdated assertions.

Sequence Diagram for Unit.__rdiv__ with a Scalar Operand

sequenceDiagram
    participant C as Caller
    participant U as unit: Unit
    participant QC as Quantity Class

    C->>U: __rdiv__(scalar_value)
    activate U
    U->>U: reverse()
    activate U
    U-->>U: inverse_unit : Unit
    deactivate U
    U->>QC: new Quantity(scalar_value, inverse_unit)
    activate QC
    QC-->>U: quantity_instance : Quantity
    deactivate QC
    U-->>C: quantity_instance : Quantity
    deactivate U
Loading

Updated Class Diagram for Unit

classDiagram
    class Unit {
        +__rdiv__(other: any) : Unit_or_Quantity
        +reverse() : Unit
    }
Loading

File-Level Changes

Change Details Files
Extracted unit inversion logic into a new reverse() method and simplified rdiv
  • Added Unit.reverse() to compute dim**-1, scale/factor inversion, and standardization
  • Updated rdiv to delegate scalar division and Quantity cases to reverse()
  • Removed legacy branch handling other==1 in rdiv
saiunit/_base.py
Adapted and pruned tests to use reverse() and removed obsolete division cases
  • Replaced expected 1/u unit assertions with u.reverse() in division tests
  • Commented out deprecated assertions checking 1/meter returns a Unit
  • Removed outdated str/repr test blocks for complex powered and combined units
saiunit/_base_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. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the 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 exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

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 - here's some feedback:

  • The new reverse() method largely duplicates the inversion logic from rdiv; consider extracting a single helper to keep the code DRY.
  • The commented-out tests for 1/meter and the large removed block in test_str_repr should be either deleted or accompanied by a clear comment explaining why they’re no longer valid.
  • If reverse() is intended as a public API you might choose a more descriptive name (e.g. invert()), or otherwise mark it as a private helper to clarify its usage.
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.

@@ -556,21 +556,21 @@ def test_multiplication_division(self):
for q in quantities:
# Scalars and array scalars
assert_quantity(q / 3, q.mantissa / 3, u)
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): Extract code out into method (extract-method)

@chaoming0625 chaoming0625 merged commit eead2ac into main Jun 2, 2025
30 checks passed
@chaoming0625 chaoming0625 deleted the fix branch June 2, 2025 03:04
ChromatinRemodeling pushed a commit to ChromatinRemodeling/saiunit that referenced this pull request Jun 12, 2025
chaoming0625 added a commit that referenced this pull request Jun 12, 2025
* Move info from setup.py to pyproject.toml

* Move more info from setup.py to pyproject.toml

* Revert numpy version change

* Fix typo

* Refactor debug_info import and update version check in wrap_init (#36)

* Refactor debug_info import and update version check in wrap_init

* Bump version to 0.0.14

* Refactor division methods in Unit class and improve reverse functionality (#38)

* Bump version to 0.0.15 and update wheel upload paths in build_wheel.sh

* Clean up pyproject.toml[.template] and setup.py[.template]

* Fix typos

* Update build_wheel.sh

---------

Co-authored-by: Tairan Li <trli@DESKTOP-F9E5SRV.localdomain>
Co-authored-by: Chaoming Wang <adaduo@outlook.com>
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