Skip to content

Conversation

@rfrenchseti
Copy link
Collaborator

@rfrenchseti rfrenchseti commented Dec 5, 2025

Overview

This PR makes a massive improvement to test coverage (from 73% to 97%), clarifies docstrings, and fixes a number of minor bugs that were found along the way.

The new test coverage is:

Name                                 Stmts   Miss Branch BrPart  Cover
----------------------------------------------------------------------
polymath/__init__.py                    15      0      0      0   100%
polymath/_version.py                     9      0      0      0   100%
polymath/boolean.py                     95      0     14      0   100%
polymath/extensions/__init__.py        167      0      0      0   100%
polymath/extensions/broadcaster.py      77      0     48      0   100%
polymath/extensions/indexer.py         308     11    196     11    95%
polymath/extensions/item_ops.py        134      0     30      0   100%
polymath/extensions/iterator.py         36      0      6      0   100%
polymath/extensions/mask_ops.py        158      0     62      0   100%
polymath/extensions/math_ops.py        803     50    382     21    93%
polymath/extensions/pickler.py         431     22    196     15    93%
polymath/extensions/shaper.py          186      0    114      0   100%
polymath/extensions/shrinker.py         86      7     48      5    91%
polymath/extensions/tvl.py             117      0     50      0   100%
polymath/extensions/vector_ops.py      266      0    132      0   100%
polymath/matrix3.py                    298      0     60      0   100%
polymath/matrix.py                     185      0     66      0   100%
polymath/pair.py                       105      0     44      0   100%
polymath/polynomial.py                 334      6    150      9    97%
polymath/quaternion.py                 356      0    104      0   100%
polymath/qube.py                      1283     31    718     30    97%
polymath/scalar.py                     778     28    402     20    96%
polymath/unit.py                       492      0    218      0   100%
polymath/vector3.py                    127      0     26      0   100%
polymath/vector.py                     371      7    168      7    97%
----------------------------------------------------------------------
TOTAL                                 7217    162   3234    118    97%

All of the changes need to be carefully reviewed, especially the changes to the code to fix apparent bugs. In some cases the intent of the code was not clear, and in other cases it just appeared to be incorrect. It is entirely possible that some of the fixes are not correct!

Per-File Change Summary

extensions/__init__.py

  • __matmul__ was missing from the copying operators, meaning that the @ operator could never work.

extensions/item_ops.py

  • Clarified docstrings.
  • Fixed copy-paste errors in exceptions.

extensions/mask_ops.py

  • In mask_where, the docstring description of recursive does not agree with the normal meaning throughout the rest of the code. Please verify the new meaning and the code changes to make it work.

extensions/math_ops.py

  • When converting arguments to Qubes, there is a group of methods that converts using as_this_type and a group that uses as_scalar. This code hasn't changed (although the docstrings have been clarified). Please make sure this is the desired behavior.
  • rsub had a bug where if passed a Qube it would return None. This looks like it was an indentation error.

The __rsub__ method (right-side subtraction, implementing arg - self) had a bug where it would implicitly return
None when the arg parameter was already a Qube object. The method had an if statement to handle the case
when arg is not a Qube, but when arg was already a Qube, the function would fall through without an explicit
return statement.

  • Docstring cleanup.

extensions/pickler.py

  • As a general hint, if flake8 says a variable is unused but you're sure it's used in an error message, make sure you have the preceding f. This happened several time in the code base.
  • Docstring cleanup.

extensions/shrinker.py

  • Docstring cleanup.

extensions/tvl.py

  • tvl_and and tvl_or did not fully document the truth table. The actual code seemed to have some subtle errors in it with masks. Please check the rewritten versions carefully.

The docstring stated:

"False and anything = False (even if False is masked or the other operand is masked)"
However, the correct interpretation (as clarified by the user) should be:
"False (unmasked) and anything = False (even if the other operand is masked)"
The original implementation checked for False values regardless of whether they were masked, which was incorrect.

Similar to tvl_and, the docstring stated:

"True or anything = True (even if True is masked or the other operand is masked)"
The correct interpretation should be:
"True (unmasked) or anything = True (even if the other operand is masked)"
The original implementation checked for True values regardless of mask.

  • Docstring cleanup.

tvl_and and tvl_or - The docstrings stated:

"For array operations, the rules apply element-wise. When operands are masked in array contexts, the result may be masked even when the simple scalar rules would suggest a definite True/False result."
This suggested that arrays had different behavior than scalars, which was incorrect.

extensions/vector_ops.py

  • The code at line 61 appears to be unreachable to I replaced it with an exception to catch any future changes.
  • The code at line 140 appears to display the wrong values.
  • The code at line 167 was nonsensical from what appears to be an indent error.
  • I made cross_2x2 and cross_3x3 internal functions.

matrix.py

  • Bugs in from_scalar.
  • Removed Python 2 support.
  • Docstring cleanup.

matrix3.py

  • poly_rotation had a bad zero argument to np.stack.

The pole_rotation() method failed when given array inputs for ra (right ascension) or dec (declination) due to a scalar literal 0. in the np.stack() call that didn't broadcast correctly with array-shaped trigonometric values.
Original Code:

values = np.stack([-sin_ra,            cos_ra,           0.,
                   -cos_ra * sin_dec, -sin_ra * sin_dec, cos_dec,
                    cos_ra * cos_dec,  sin_ra * cos_dec, sin_dec],
                  axis=-1)

Error:

ValueError: all input arrays must have the same shape

This occurred when ra or dec were arrays (e.g., np.array([0., np.pi/4])), causing sin_ra and cos_ra to be arrays, while 0. remained a scalar. NumPy's np.stack() requires all inputs to have the same shape.

  • __imul__ called _set_values with a bad example argument.
  • from_euler and to_euler did not work with a tuple of axes because lower was called on the tuple before it was detected.
  • Removed Python 2 support.
  • Docstring cleanup.

pair.py

  • from_scalars claimed to support None arguments but didn't.

The original Pair.from_scalars() method did not handle None values for the x and y parameters. When None was > passed, the code would fail with:

TypeError: invalid Scalar data type: <class 'NoneType'>

This occurred because Qube.from_scalars() (which was called directly) attempted to convert None to a Scalar, which is not a valid conversion.

  • rot90 needed to pass recursive as a named argument.

The rot90() method had a bug when handling derivatives. The code attempted to call:

obj.insert_deriv(key, deriv.rot90(False))

However, rot90() has a keyword-only parameter recursive, so passing False as a positional argument caused:

TypeError: Pair.rot90() takes 1 positional argument but 2 were given
  • Removed Python 2 support.
  • Docstring cleanup.

polynomial.py

  • The handling of derivatives in invert_line is highly questionable and needs to be validated.
  • For __iadd__ and __isub__ this is not valid Python: self.super().__iadd__(arg.super()). Cursor wrote a fix to handle polynomials of different order, but it needs to be validated and there's probably a way to simplify it.

The in-place addition and subtraction methods (__iadd__ and __isub__) were using the anti-pattern self = ..., which creates a new local variable instead of modifying the existing object. This violates the contract of in-place operations.

The code called self._add_derivs(self, arg) and self._sub_derivs(self, arg), but these are class methods that expect two arguments. The correct call should use Qube._add_derivs(self, arg) and Qube._sub_derivs(self, arg).

The code contained an unreachable branch in the conditional logic for handling polynomial order alignment. The else branch (arg.set_order(max_order)) was mathematically impossible to execute.

  • Likewise in __sub__ and __rsub__ this is not valid Python: return Polynomial(self.super() - arg.super()) and was replaced with Polynomial(self.as_vector() - arg.as_vector()).
  • __mul__ had some problems and was rewritten.
  • __pow__ did not work at all and was rewritten.
  • __eval__ and roots had a lot of issues and were significantly changed.

The eval() method had a bug when extracting constant values from polynomials with denominators (drank > 0). The > original code used:

const_values = self._values[(Ellipsis, 0) + tail_indx]

This failed because tail_indx is a tuple of slices, and tuple concatenation with + doesn't work correctly in this context > when used for NumPy indexing.

For zero-order (constant) polynomials, the code attempted to create a Scalar using Scalar(example=self) without providing the required arg parameter. The Scalar constructor requires an arg parameter as the first positional argument.

When total_shifts is an array (not a scalar), the code attempted to use boolean indexing incorrectly: root_mask[total_shifts > k, k]. This fails because the boolean array shape doesn't match the root_mask shape for advanced indexing.

The duplicate detection logic was applied after calling roots.sort(), which converts masked values to inf. This makes it impossible to detect duplicates because masked values are no longer comparable.

The code called self.deriv.eval(...) but deriv is a method, not a property, so it needs to be called first: self.deriv().eval(...).

In the scalar case of the while np.any(shifts) loop, the code was using break after shifting once, which would exit the loop even if the leading coefficient was still zero. This could leave leading zeros in the coefficients array, causing np.linalg.eigvals to fail.

  • Docstring cleanup.

quaternion.py

  • to_matrix3 didn't support scalar pnorms.

The code attempted to handle scalar pnorm values (when the quaternion has shape ()) but had two issues:

  1. It checked np.shape(pvals) == () instead of np.shape(pnorm) == () when determining if pnorm was scalar
  2. When pnorm was scalar, the code tried to use it directly in array operations with pnorm[..., np.newaxis], which would fail because scalar NumPy values don't support indexing
  • from_euler did not work with a tuple of axes because lower was called on the tuple before it was detected.
  • Removed Python 2 support.
  • Docstring cleanup.

qube.py

  • _suitable_dtype missing f string prefix.
  • delete_derives modified a dict that was being iterated over.
  • without_unit didn't remove units from derivatives.

The docstring stated:

"If recursive is True, derivatives are also stripped of their units."
However, the implementation did not actually strip units from derivatives when recursive=True.

  • as_float called __copy__ with a recursive parameter but that parameter is not supported by __copy__.
  • as_size_zero was not operating properly with scalars.

Fixed bug in polymath/qube.py line 2621: When as_size_zero is called with axis=0 and the mask is a scalar, the mask
shape was (0,) but should match the values shape (0, 2). Fixed by creating a mask array that matches the shape of
new_values.

  • Docstring cleanup.

into_unit - The docstring stated:

"Converted to its unit"
This was ambiguous about the direction of conversion - does it convert TO the unit or FROM the unit?

scalar.py

  • sort did not properly replace masked values with the maximum.

The code attempted to assign result.max() directly to an array slice, but result.max() returns a Scalar object, not a raw NumPy value.

unit.py

  • mul_units and div_units. I really did not understand these methods. There is an argument name that was ignored entirely. If a new unit was created, its name was set to None, but if one of the original units was returned, it kept its name. I cleaned up the logic and made it set the new unit's name to name, but the overall intent of this function needs to be verified.
  • name_to_dict wraps parsing in at 99,999 characters in a way that can't possibly work. I don't understand the purpose of the wrap. If it's a limit, we should just throw an exception. This code needs to be verified.
  • Docstring cleanup.

vector.py

  • Removed Python 2 support.
  • Docstring cleanup.

vector3.py

  • from_scalars claimed to support None arguments but didn't. Same as pair.py.
  • Removed Python 2 support.
  • Docstring cleanup.

pyproject.toml

  • Removed support for Python 3.8 and 3.9.

New Test Files

  • There are roughly 16,000 lines of new test cases.
  • Because some of these test files were produced from scratch without regards to what was already being tested, it is likely there are multiple tests that cover the same functions and lines. But I don't think this is a problem. But tests still run in about 13 seconds and more ways of testing a function is always good.

Summary by CodeRabbit

  • Python Version Update

    • Minimum Python requirement raised to 3.10.
  • New Features

    • Matrix multiplication via matmul for qube-like objects.
    • Euler-angle inputs for quaternions accept tuple/list axes as well as strings.
  • Public API

    • into_unit now enforces keyword-only usage (recursive keyword).
  • Documentation

    • Extensive docstring clarifications across core numeric types and operations.
  • Bug Fixes / Behavior

    • Tighter shape validation, improved masking, derivative propagation, unit handling, and sort behavior.
  • Tests

    • Large expansion of unit tests covering many classes and edge cases.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 5, 2025

Walkthrough

Removed legacy __future__ imports and raised Python requirement to >=3.10; tightened broadcasting and None handling in several constructors; substantial internal rewrites (Polynomial, operator coercion, masking/tvl, vector ops); Qube/unit API refinements; added Qube.__matmul__ binding; and added/expanded ~30+ test modules covering broad functionality and edge cases.

Changes

Cohort / File(s) Summary
Project metadata & import cleanup
pyproject.toml, polymath/boolean.py, polymath/vector.py, polymath/pair.py
Bumped required Python to >=3.10; removed obsolete __future__ division imports; small docstring and error-message tweaks.
Polynomial core rewrite
polymath/polynomial.py
Large internal rewrite: in-place and binary arithmetic, convolution-based multiplication, repeated-squaring power, eval/roots, coefficient-order/shape/mask/derivative state changes and propagation.
Matrix & Matrix3 updates
polymath/matrix.py, polymath/matrix3.py
from_scalars count validation tightened; doc/shape checks improved; Matrix3: broadcast-compatible stacking, axis handling broadened, doc updates, and unreachable branches annotated with pragma:no cover.
Vector & Vector3
polymath/vector.py, polymath/vector3.py
Doc clarifications; Vector3.from_scalars substitutes None with zero Scalars using broadcasting logic; many coordinate/angle/cylindrical doc updates; minor behavioral notes.
Pair adjustments
polymath/pair.py
from_scalars converts non-None inputs to Scalars, infers broadcasting/denominator exemplar, substitutes None with zeros; minor derivative-call signature fix in rot90.
Quaternion updates
polymath/quaternion.py
Doc/runtime clarifications, shape-aware zero-masking, from_euler accepts tuple/list axes, and some pragma:no cover annotations; no public-signature changes.
Scalar change
polymath/scalar.py
sort now fills masked positions with a dtype-specific max_possible constant instead of using an intermediate result.max().
Qube core & unit handling
polymath/qube.py, polymath/unit.py
Qube.into_unit made keyword-only (*, recursive=False); safer iteration when mutating derivatives; copy/shape/mask refinements; Unit uses internal name map, adds explicit None-handling for mul/div, and wrapper helpers sqrt_unit/unit_power.
Extensions — math & operator coercion
polymath/extensions/math_ops.py
Operators now coerce non-Qube args to appropriate Qube/Scalar types (as_this_type/as_scalar) with scalar fast-paths and adjusted in-place behaviors; docstrings expanded.
Extensions — item/mask/vector/tvl/pickler/shrinker
polymath/extensions/item_ops.py, polymath/extensions/mask_ops.py, polymath/extensions/vector_ops.py, polymath/extensions/tvl.py, polymath/extensions/pickler.py, polymath/extensions/shrinker.py
Doc/error-message fixes; mask_where now respects recursive; internal _mask usage; renamed helpers cross_3x3_cross_3x3, cross_2x2_cross_2x2; refined three-valued-logic semantics in tvl_and/tvl_or; pickler docs and error formatting updated; shrink/unshrink docs clarified.
Extensions — operator binding
polymath/extensions/__init__.py
Added binding: Qube.__matmul__ = item_ops.__matmul__.
Tests — additions & expansions
tests/... (many files; examples) tests/test_matrix3.py, tests/test_quaternion.py, tests/test_pair.py, tests/test_polynomial_*.py, tests/test_qube_*.py, tests/test_scalar_*.py, tests/test_vector_*.py, tests/test_matrix_*.py, tests/test_units.py, tests/test_math_ops_coverage.py, tests/test_indices.py, tests/test_boolean.py, tests/test_vector_int.py, ...
Added or expanded ~30+ test modules with broad coverage for Matrix/Matrix3, Quaternion, Vector/Vector3, Pair, Polynomial, Qube core and extensions (item/mask/math/shrinker/tvl/pickler), Scalar, Matrix, Unit, reshaping, indexing, and math operator behaviors. Tests exercise broadcasting, masking, derivative propagation, and numerous edge cases.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Areas to focus review on:

  • polymath/polynomial.py — dense arithmetic, convolution/pow logic, eval/roots, derivative propagation.
  • polymath/extensions/math_ops.py — widespread coercion changes and scalar fast-paths across many operators.
  • polymath/extensions/tvl.py and polymath/extensions/mask_ops.py — masking semantics and truth-propagation logic.
  • polymath/matrix.py, polymath/matrix3.py, polymath/vector3.py, polymath/pair.py — broadcasting, None→zero substitution, and shape/count validation.
  • polymath/qube.py and polymath/unit.py — keyword-only signature change, derivative mutation safety, and unit helper additions.
  • Test additions — large volume of new/updated tests; verify they match intended behavior and do not lock in undesired edge-case semantics.

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main focus of the PR: significantly increasing test coverage (73% to 97%), improving docstrings across the codebase, and implementing targeted bug fixes. While comprehensive in scope, it clearly communicates the primary change from the developer's perspective.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 940d777 and 45d7e53.

📒 Files selected for processing (1)
  • polymath/extensions/math_ops.py (21 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
polymath/extensions/math_ops.py

299-299: Missing return type annotation for private function __isub__

(ANN202)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Test polymath (windows-latest, 3.8)
  • GitHub Check: Test polymath (macos-latest, 3.8)
🔇 Additional comments (4)
polymath/extensions/math_ops.py (4)

88-91: LGTM! Clear documentation of argument coercion.

The updated docstrings accurately describe how non-Qube arguments are converted using as_this_type() and note the scalar optimization path for efficiency.

Also applies to: 141-142


230-233: LGTM! Docstrings accurately describe argument handling.

The subtraction operator docstrings correctly document the conversion behavior and optimization paths.

Also applies to: 303-304


379-382: LGTM! Docstrings now accurately reflect Scalar coercion behavior.

These docstring updates correctly document that non-Qube arguments are converted to Scalar via Qube._SCALAR_CLASS.as_scalar() for multiplication, division, modulo, and power operations. This resolves the inconsistency noted in the previous review.

Based on past review comments, this addresses the previously identified documentation-code mismatch.

Also applies to: 433-434, 462-463, 592-595, 649-650, 680-681, 805-809, 847-851, 873-874, 963-964, 1010-1011, 1038-1039, 1127-1139


1449-1456: LGTM! Excellent documentation improvements.

The enhanced docstrings add valuable clarity:

  • __bool__ now clearly explains the _truth_if_all and _truth_if_any behavior for equality/inequality comparisons and their interaction with masking.
  • mean() includes helpful examples showing how axis arguments affect result shapes, making the API more discoverable.

Also applies to: 1902-1922


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 97.56098% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.11%. Comparing base (7e4a15f) to head (45d7e53).

Files with missing lines Patch % Lines
polymath/polynomial.py 95.94% 2 Missing and 4 partials ⚠️
polymath/qube.py 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #13       +/-   ##
===========================================
+ Coverage   72.83%   96.11%   +23.28%     
===========================================
  Files          24       24               
  Lines        7123     7207       +84     
  Branches     1599     1617       +18     
===========================================
+ Hits         5188     6927     +1739     
+ Misses       1497      162     -1335     
+ Partials      438      118      -320     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
polymath/polynomial.py (4)

105-115: as_vector reuses self instead of each derivative when building _derivs

In as_vector, the recursive branch currently does:

derivs = {}
if recursive:
    for key, value in self._derivs.items():
        derivs[key] = self.as_vector(recursive=False)

This means every derivative entry ends up pointing at the same vectorized copy of self, not the corresponding derivative polynomial (value). The resulting Vector has incorrect derivative content.

A minimal fix is to convert each derivative value instead:

-        derivs = {}
-        if recursive:
-            for key, value in self._derivs.items():
-                derivs[key] = self.as_vector(recursive=False)
-
-        obj.insert_derivs(derivs)
+        derivs = {}
+        if recursive:
+            for key, value in self._derivs.items():
+                # Preserve derivative structure but convert each Polynomial derivative
+                # to a plain Vector.
+                derivs[key] = value.as_vector(recursive=False)
+
+        obj.insert_derivs(derivs)

This keeps the existing API while making derivative propagation through as_vector logically correct.


373-382: __mul__ denominator handling and docstring are inconsistent

The docstring says a ValueError is raised “when self._drank != arg._drank and both are non‑zero”, but the implementation raises whenever _drank differs, including mixing a denominator‐bearing polynomial with one that has drank == 0:

if isinstance(arg, Polynomial):
    if self._drank != arg._drank:
        raise ValueError('incompatible denominators for multiply')

Tests (tests/test_polynomial.py, p_mul_drank1 *__mul__ p_mul_drank2) explicitly expect the error for drank=1 times drank=0, so behavior is fine; the docstring is now misleading.

I’d suggest updating the docstring to match reality, e.g.:

-        Raises:
-            ValueError: If the polynomials have incompatible denominators. This
-                occurs when self._drank != arg._drank and both are non-zero. For example,
-                a polynomial with drank=1 cannot be multiplied by a polynomial with
-                drank=2.
+        Raises:
+            ValueError: If the polynomials have incompatible denominators, i.e.
+                when ``self._drank != arg._drank``. Mixing polynomials with and
+                without denominators, or with different denominator ranks, is not
+                supported.

505-525: __pow__ returns the original object for exponent 1 (aliasing)

The repeated‑squaring implementation initializes result = None and then:

while power > 0:
    if power % 2 == 1:
        if result is None:
            result = base
        else:
            result = result * base
    base = base * base
    power //= 2

For arg == 1, this returns result is self rather than a new Polynomial. That means callers who later mutate the result (e.g. via in‑place operators) will accidentally mutate the original polynomial, unlike the arg >= 2 cases which always build new instances via *.

If you want p ** 1 to behave like a pure expression, consider starting from an identity polynomial and always multiplying:

-        # Use repeated squaring algorithm
-        result = None
+        # Use repeated-squaring algorithm starting from the multiplicative identity
+        result = Polynomial([1.])
         base = self
         power = int(arg)

so that even arg == 1 returns a distinct object (1 * base) instead of aliasing self.


672-783: roots() doesn't account for _drank and will fail for polynomials with denominators

The higher-order roots() logic (all-zeros detection, leading-zero shifts, companion matrix, and masking) hard-codes axis operations that assume the coefficient axis is at -1:

all_zeros = np.all(coefficients == 0., axis=-1)
shifts = (coefficients[..., 0] == 0.)
coefficients[shifts, :-1] = coefficients[shifts, 1:]
matrix[..., 0, :] = -coefficients[..., 1:] / coefficients[..., 0:1]

However, the polynomial's order property reveals that the coefficient axis is actually at -self._drank - 1:

return self.item[-self._drank - 1] - 1

This means:

  • When _drank == 0: coefficient axis is at -1 ✓ (current code works)
  • When _drank > 0: coefficient axis is at -2, -3, etc., but the code still uses -1 ✗ (indexes denominator axes instead)

For polynomials with denominators, operations like all_zeros = np.all(coefficients == 0., axis=-1) and shifts = (coefficients[..., 0] == 0.) will act on the wrong axes, causing shape mismatches, boolean indexing errors, or incorrect masking. The linear and quadratic cases use to_scalars() which correctly extracts coefficients via numerator indexing, but the higher-order path is broken. Tests cover roots() only for _drank == 0.

Recommendation: Either explicitly disallow denominators with a check like if self._drank: raise ValueError("Polynomial.roots() does not support denominators (drank > 0) yet"), or refactor to use coef_axis = -self._drank - 1 consistently throughout the higher-order implementation and add test coverage for _drank > 0.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 9

♻️ Duplicate comments (2)
tests/test_matrix3.py (1)

664-666: Remove unused exception variable e.

The exception handler assigns as e but never uses it.

polymath/polynomial.py (1)

253-286: State recomputation duplicated between __iadd__ and __isub__

The manual state recomputation logic (lines 261-275) is duplicated in __isub__ (lines 335-349). As noted in a previous review, this is brittle and should be factored into a helper method.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

♻️ Duplicate comments (7)
polymath/polynomial.py (5)

812-817: Scalar duplicate detection may miss multiple duplicates.

This was noted in a previous review. For scalar polynomials with triple roots or multiple duplicate pairs, the current logic breaks after detecting the first duplicate. If this is intentional (e.g., scalar polynomials are expected to have at most one duplicate pair), consider adding a comment to clarify this assumption.


253-287: State recomputation pattern is acceptable but consider Ruff's suggestion.

The approach of using a temporary Polynomial to compute invariants is reasonable. As a minor style improvement, consider using tuple unpacking as suggested by Ruff:

-            new_values = np.zeros(self._shape + (max_order+1,))
+            new_values = np.zeros((*self._shape, max_order+1))

420-424: Minor style: prefer tuple unpacking for index construction.

The indexing logic is correct. As a readability improvement per Ruff's RUF005 suggestion:

-                    self_indx = (Ellipsis, i) + suffix
-                    arg_indx = (Ellipsis, j) + suffix
-                    result_indx = (Ellipsis, k) + suffix
+                    self_indx = (Ellipsis, i, *suffix)
+                    arg_indx = (Ellipsis, j, *suffix)
+                    result_indx = (Ellipsis, k, *suffix)

630-631: Replace assert with explicit error handling in production code.

The assert deriv.order == 0 at line 631 could cause an AssertionError in production if a polynomial with non-zero order derivatives is passed. This was flagged in a previous review.

-                    assert deriv.order == 0
+                    if deriv.order != 0:
+                        raise ValueError('Derivative of constant polynomial must be constant')

666-668: Rename unused loop variable k to _k.

The loop variable k is not used within the loop body, as flagged by Ruff (B007).

-        for k in range(1, self.order):
+        for _k in range(1, self.order):
             x_power = x_power * x  # Create new object, don't modify in place
             x_powers.append(x_power)
tests/test_polynomial_basic.py (1)

102-103: Comment inconsistency with test polynomial.

The comment mentions "y = 2x + 3" but the test constructs Polynomial([3., 2.]) which represents 3x + 2. This was noted in a previous review.

-        # Derivative of inverse: if y = 2x + 3, then x = 0.5y - 1.5
-        # If dy/dt = 2, then dx/dt = 0.5 * 2 = 1
+        # Derivative of inverse: if y = 3x + 2, then x = (1/3)y - 2/3
         # But we need to check the actual derivative structure
tests/test_pair.py (1)

12-16: Consider replacing a single runTest with focused test_* methods

This mirrors earlier feedback: packing the entire Pair API coverage into one runTest makes failures harder to localize and limits pytest’s ability to select individual scenarios. Splitting into logically grouped test_* methods (construction, as_pair, from_scalars, swapxy/rot90, angle, clip2d, etc.) would improve isolation and reporting, while keeping the actual assertions the same.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e2f5f8 and b363fe3.

📒 Files selected for processing (6)
  • polymath/pair.py (3 hunks)
  • polymath/polynomial.py (16 hunks)
  • polymath/vector3.py (12 hunks)
  • tests/test_pair.py (1 hunks)
  • tests/test_polynomial_arithmetic.py (1 hunks)
  • tests/test_polynomial_basic.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
polymath/vector3.py (3)
polymath/extensions/math_ops.py (2)
  • len (75-78)
  • zero (1791-1818)
polymath/scalar.py (2)
  • Scalar (19-1892)
  • as_scalar (77-103)
polymath/qube.py (1)
  • Qube (11-3144)
polymath/pair.py (1)
polymath/qube.py (2)
  • Qube (11-3144)
  • insert_deriv (1506-1572)
🪛 Ruff (0.14.7)
tests/test_pair.py

20-20: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


21-21: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


22-22: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


23-23: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


27-27: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


31-31: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


35-35: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


39-39: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


40-40: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


41-41: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


45-45: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


46-46: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


47-47: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


50-50: Use pytest.raises instead of unittest-style assertRaises

(PT027)


51-51: Use pytest.raises instead of unittest-style assertRaises

(PT027)


52-52: Use pytest.raises instead of unittest-style assertRaises

(PT027)


53-53: Use pytest.raises instead of unittest-style assertRaises

(PT027)


57-57: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


58-58: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


59-59: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


60-60: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


63-63: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


64-64: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


65-65: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


66-66: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


69-69: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


70-70: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


71-71: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


72-72: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


75-75: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


76-76: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


77-77: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


79-79: Use pytest.raises instead of unittest-style assertRaises

(PT027)


83-83: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


84-84: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


85-85: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


86-86: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


89-89: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


90-90: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


91-91: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


92-92: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


96-96: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


97-97: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


98-98: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


101-101: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


102-102: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


103-103: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


104-104: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


109-109: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


110-110: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


115-115: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


116-116: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


120-120: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


121-121: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


125-125: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


127-127: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


128-128: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


132-132: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


134-134: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


135-135: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


139-139: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


140-140: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


142-142: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


143-143: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


144-144: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


145-145: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


150-150: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


151-151: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


152-152: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


153-153: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


155-155: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


158-158: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


159-159: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


160-160: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


161-161: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


165-165: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


166-166: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


172-172: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


173-173: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


174-174: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


180-180: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


181-181: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


182-182: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


188-188: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


189-189: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


190-190: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


194-194: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


198-198: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


201-201: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


206-206: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


207-207: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


209-209: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


213-213: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


214-214: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


215-215: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


222-222: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


223-223: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


225-225: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


226-226: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


227-227: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


228-228: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


233-233: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


239-239: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


240-240: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


245-245: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


246-246: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


247-247: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


253-253: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


254-254: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


255-255: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


261-261: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


262-262: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


263-263: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


264-264: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


269-269: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


271-271: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


277-277: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


282-282: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


283-283: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


284-284: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


290-290: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


291-291: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


292-292: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


298-298: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


299-299: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


300-300: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


302-302: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


307-307: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


308-308: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


312-312: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


317-317: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


318-318: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


319-319: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


324-324: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


325-325: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


327-327: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


333-333: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


334-334: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


341-341: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


343-343: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


349-349: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


351-351: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


357-357: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


359-359: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


366-366: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


368-368: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


369-369: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


377-377: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


379-379: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


386-386: Use pytest.raises instead of unittest-style assertRaises

(PT027)


392-392: Use pytest.raises instead of unittest-style assertRaises

(PT027)


399-399: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


401-401: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


408-408: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


410-410: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


417-417: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


419-419: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


424-424: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


425-425: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


429-429: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


430-430: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


431-431: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


437-437: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


439-439: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


446-446: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


451-451: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


453-453: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


458-458: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


463-463: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


465-465: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


466-466: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


471-471: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


474-474: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


475-475: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


476-476: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


478-478: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


479-479: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


480-480: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


482-482: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


483-483: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


484-484: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


486-486: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


487-487: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


488-488: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


490-490: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


491-491: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


492-492: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


494-494: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


495-495: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


496-496: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


498-498: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


499-499: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


500-500: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


502-502: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


503-503: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


504-504: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


505-505: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


506-506: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


508-508: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


509-509: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


510-510: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


512-512: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


513-513: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


514-514: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


518-518: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


521-521: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


525-525: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


528-528: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


532-532: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


533-533: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


534-534: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


540-540: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


543-543: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


548-548: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


553-553: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


564-564: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)

polymath/polynomial.py

259-259: Consider (*self._shape, max_order + 1) instead of concatenation

Replace with (*self._shape, max_order + 1)

(RUF005)


333-333: Consider (*self._shape, max_order + 1) instead of concatenation

Replace with (*self._shape, max_order + 1)

(RUF005)


420-420: Consider (Ellipsis, i, *suffix) instead of concatenation

Replace with (Ellipsis, i, *suffix)

(RUF005)


421-421: Consider (Ellipsis, j, *suffix) instead of concatenation

Replace with (Ellipsis, j, *suffix)

(RUF005)


422-422: Consider (Ellipsis, k, *suffix) instead of concatenation

Replace with (Ellipsis, k, *suffix)

(RUF005)


634-634: Consider (Ellipsis, 0, *deriv_tail) instead of concatenation

Replace with (Ellipsis, 0, *deriv_tail)

(RUF005)


644-645: Consider (Ellipsis, 0, *dvalue_tail) instead of concatenation

Replace with (Ellipsis, 0, *dvalue_tail)

(RUF005)


666-666: Loop control variable k not used within loop body

Rename unused k to _k

(B007)


797-797: Consider (k, *mask_indices) instead of concatenation

Replace with (k, *mask_indices)

(RUF005)

tests/test_polynomial_arithmetic.py

21-21: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


22-22: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


29-29: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


30-30: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


32-32: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


33-33: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


34-34: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


39-39: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


40-40: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


41-41: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


46-46: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


47-47: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


53-53: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


54-54: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


56-56: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


57-57: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


58-58: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


63-63: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


64-64: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


65-65: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


70-70: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


71-71: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


78-78: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


79-79: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


81-81: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


82-82: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


84-84: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


85-85: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


86-86: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


91-91: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


92-92: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


97-97: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


98-98: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


104-104: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


105-105: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


107-107: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


108-108: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


109-109: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


113-113: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


114-114: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


115-115: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


116-116: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


117-117: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


122-122: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


123-123: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


124-124: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


127-127: Use pytest.raises instead of unittest-style assertRaises

(PT027)


128-128: Use pytest.raises instead of unittest-style assertRaises

(PT027)


134-134: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


135-135: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


136-136: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


137-137: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


148-148: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


155-155: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


156-156: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


157-157: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


162-162: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


163-163: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


164-164: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


173-173: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


181-181: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


187-187: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


188-188: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


189-189: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


195-195: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


196-196: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


197-197: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


202-202: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


203-203: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


204-204: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


211-211: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


214-214: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


222-222: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


228-228: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


237-237: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


243-243: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


251-251: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


261-261: Use pytest.raises instead of unittest-style assertRaises

(PT027)


267-267: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


268-268: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


274-274: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


278-278: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


279-279: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


289-289: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


291-291: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


292-292: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


302-302: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


303-303: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)

tests/test_polynomial_basic.py

22-22: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


23-23: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


24-24: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


29-29: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


30-30: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


34-34: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


37-37: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


40-40: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


44-44: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


45-45: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


50-50: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


51-51: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


56-56: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


57-57: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


62-62: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


63-63: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


65-65: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


66-66: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


68-68: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


69-69: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


74-74: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


75-75: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


80-80: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


81-81: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


85-85: Use pytest.raises instead of unittest-style assertRaises

(PT027)


91-91: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


93-93: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


94-94: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


101-101: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


105-105: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


109-109: Use pytest.raises instead of unittest-style assertRaises

(PT027)


115-115: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


121-121: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


122-122: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


123-123: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


125-125: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


127-127: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


141-141: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


143-143: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


149-149: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


152-152: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


159-159: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


165-165: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


171-171: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


172-172: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


178-178: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


180-180: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Test polymath (windows-latest, 3.10)
  • GitHub Check: Test polymath (macos-latest, 3.8)
  • GitHub Check: Test polymath (windows-latest, 3.8)
  • GitHub Check: Test polymath (macos-latest, 3.11)
🔇 Additional comments (6)
polymath/vector3.py (2)

41-47: LGTM! Excellent documentation improvements.

The expanded docstrings significantly improve API clarity by specifying:

  • Conversion behaviors and supported input forms
  • Coordinate system conventions and angle ranges
  • Return value semantics and units
  • None-handling behavior

These enhancements align well with the PR's goal of improving documentation alongside test coverage.

Also applies to: 85-88, 133-146, 171-174, 191-205, 224-227, 245-246, 260-263, 303-307, 339-343


91-122: The unit compatibility concern in this review is incorrect. The implementation properly preserves units through the example parameter in the zero() method. When zero() calls obj.__init__(new_value, False, derivs={}, example=self), the Qube.__init__ method copies the unit from the example object (see qube.py lines 202-203: if unit is None and self._UNITS_OK: unit = example._unit). Existing tests in test_vector3_basic.py (lines 187-194) confirm the implementation correctly handles complex cases like mixed None with Scalars that have denominators, which is more complex than unit preservation alone.

polymath/polynomial.py (1)

528-542: LGTM!

The repeated squaring algorithm is correctly implemented and provides efficient O(log n) multiplication operations for computing polynomial powers.

polymath/pair.py (2)

181-185: Correct fix for keyword-only recursive in rot90 derivatives

Switching to deriv.rot90(recursive=False) is necessary and sufficient here: the method’s signature is keyword‑only, and this now cleanly reuses the same rotation logic for derivatives without accidentally recursing their own derivs.


214-215: clip2d remask docstring now matches intent

The updated wording (“True to keep the mask; False to replace the values but make them unmasked”) is clearer and lines up with forwarding remask through to clip_component without changing behavior.

tests/test_pair.py (1)

203-215: Good coverage of from_scalars with None and n‑D denominators

These tests nicely pin down the intended behavior for Pair.from_scalars when mixing Scalar inputs with denominators and None (including the all‑None case). They should catch regressions in denominator propagation and zero‑filling semantics going forward.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 32

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (13)
polymath/extensions/pickler.py (1)

743-774: Lossiness note mentions impossible reference == "double" value

The new notes in __getstate__/__setstate__ say lossy compression occurs “e.g., when digits < 16 or reference != "double"”, but "double" is not a valid reference option (allowed values are e.g. "fpzip", "smallest", etc.). This is likely meant to refer to:

  • digits != "double" (or integer digits below full precision), and/or
  • reference != "fpzip".

Please correct the wording so it matches the actual digits/reference API and when fpzip is lossless vs lossy.

Also applies to: 888-902

polymath/unit.py (2)

99-122: as_unit behavior and errors unchanged but still return KeyError

Switching to Unit._NAME_TO_UNIT[arg] keeps behavior the same (still raises KeyError for unknown names even though the docstring says ValueError). If you want the docstring to be accurate, consider wrapping the lookup and raising ValueError instead; otherwise, update the docstring to mention KeyError.


529-553: mul_units/div_units ignore the name parameter and always clear names

The new None handling is reasonable, but name is now unused and result.name is always set to None, so callers cannot assign a custom name even though the signature and docstring still advertise it.

Either:

  • Document that name is ignored and names are always derived automatically, or
  • Restore honoring name (e.g., by calling result.set_name(name) when provided).

Right now the API surface is misleading.

Also applies to: 569-579

polymath/vector.py (2)

27-41: Minor docstring typo in Vector.__init__

"arguments passd to the Qube constructor" should be "arguments passed to the Qube constructor".


619-645: Fix incorrect attribute access in cross_product_as_matrix: use .shape instead of ._shape

At lines 630 and 644, the code incorrectly accesses ._shape on NumPy arrays. Since self._values and new_values are NumPy ndarrays, the correct attribute is .shape:

Line 630:

old_values = np.rollaxis(self._values, -self._drank - 1,
                         len(self._values.shape))

Line 644:

new_values = np.rollaxis(new_values, -3, len(new_values.shape))

This bug will raise AttributeError whenever self._drank > 0.

polymath/extensions/vector_ops.py (1)

156-168: Simplify _zero_sized_result loop/else structure

The for-else block

if isinstance(axis, (list, tuple)):
    for i in axis:
        indx[i] = 0
    else:
        indx[i] = 0
else:
    indx[axis] = 0

executes indx[i] = 0 twice for the last i and makes the control flow harder to read. It’s equivalent to:

if isinstance(axis, (list, tuple)):
    for i in axis:
        indx[i] = 0
else:
    indx[axis] = 0

Consider removing the for ... else to avoid confusion.

polymath/polynomial.py (4)

243-287: __iadd__ bypasses unit checks and assumes _drank == 0 for padding

The in-place add currently:

  • Pads self by allocating new_values = np.zeros(self._shape + (max_order+1,)) and assigning new_values[..., -self.order-1:] = self._values, which ignores any denominator axes; this only works safely when _drank == 0.
  • Combines units via self._unit = self._unit or arg._unit without checking for compatibility, whereas __add__ goes through Vector.__add__/Qube.__add__ which enforce unit compatibility.

To avoid silent inconsistencies:

  • Either restrict __iadd__ to _drank == 0 with an explicit check, or include _denom when forming new_values and its indices.
  • Validate units as Qube.__iadd__ does, e.g. by performing the operation on the underlying Vectors and copying the result back, or by calling a helper that wraps the standard add logic instead of reimplementing it.

As-is, you can add polynomials with incompatible units and (if denominators are ever non-zero) risk shape/indexing errors.


317-361: __isub__ has the same unit and denominator-shape issues as __iadd__

The in-place subtraction mirrors __iadd__:

  • Uses new_values = np.zeros(self._shape + (max_order+1,)) and new_values[..., -self.order-1:] = self._values, ignoring denominator axes when _drank > 0.
  • Sets self._unit = self._unit or arg._unit without checking compatibility.
  • Rebuilds derivatives via _sub_derivs(self, arg) but bypasses any higher-level checks present in Qube.__isub__.

It should be refactored in tandem with __iadd__ to reuse a single helper that:

  • Pads coefficients while respecting _denom layout, and
  • Delegates to the standard add/sub machinery (or at least reproduces its unit checks) before mutating self.

363-428: Polynomial __mul__ assumes no denominators in the result array

The convolution logic correctly identifies the coefficient axis on the inputs via:

coef_axis = -self._drank - 1
nself = self._values.shape[coef_axis_pos]
narg  = arg._values.shape[coef_axis_pos]

but it builds new_values as:

new_values = np.zeros(new_shape + (new_order + 1,))
...
suffix = self._drank * (slice(None),)
...
result_indx = (Ellipsis, k) + suffix
new_values[result_indx] += self._values[self_indx] * arg._values[arg_indx]

If _drank > 0, suffix adds denominator slices that new_values doesn’t have, so indexing new_values[result_indx] will be inconsistent with its shape. You also only check _drank equality, not that denominator shapes match.

If polynomials with denominators are supported, new_values must include _denom in its shape (and the indices adjusted accordingly). If they are not, consider asserting _drank == 0 early to fail fast with a clear error instead of relying on incidental IndexErrors.


602-675: Zero-order eval now works but ignores broadcasting with x

The new self.order == 0 branch correctly extracts the constant coefficient and builds a Scalar with derivatives, fixing the old failure for constant polynomials.

However, the result’s shape is determined solely from self._values (via const_values and example=self / self.wod) and does not depend on x at all. The docstring now states that self and x are broadcast together; that is true for higher-order polynomials (the dot-product path), but not for the constant case.

If you want shape behavior to be uniform, consider broadcasting const_values against x (e.g., via Scalar.as_scalar(x).broadcast_to(...)) or at least clarifying in the docstring that for order-zero polynomials the output shape is that of the polynomial, independent of x.

polymath/extensions/mask_ops.py (1)

43-47: Consider passing recursive through as_this_type() for replacement values

mask_where now honors the caller’s recursive choice for copies and remasking, but the replacement is still normalized via:

replace = self.as_this_type(replace, recursive=True)

Even though the final object is created with copy(recursive=recursive), this forces the replacement value to carry all of its derivatives, regardless of the caller’s intent. Depending on how __setitem__ is implemented, derivatives on replace could still influence behavior when recursive=False.

To keep semantics fully consistent and easier to reason about, consider:

-    if replace is not None:
-        replace = self.as_this_type(replace, recursive=True)
+    if replace is not None:
+        replace = self.as_this_type(replace, recursive=recursive)

This way, a caller who explicitly opts out of recursive derivative handling gets a result built entirely from non‑recursive inputs.

Also applies to: 72-76

polymath/extensions/math_ops.py (1)

1211-1226: Fix __ipow__ unit update: wrong arguments passed into set_unit

__ipow__ currently does:

result = self ** arg
self._set_values(result._values, result._mask)
self.set_unit(self, result._unit)

Given Qube.set_unit(self, unit, *, override=False) and the snippet from qube.py, this is problematic:

  • The bound call self.set_unit(self, result._unit) passes the instance as the unit argument and result._unit as override, so unit is a Qube, not a Unit/str/None.
  • Inside set_unit, Unit.as_unit(unit) will see a Qube and raise ValueError("not a recognized unit: ..."), breaking in-place power for callers relying on this generic implementation.

You likely just want to adopt the unit from result. A minimal fix:

-    result = self ** arg
-    self._set_values(result._values, result._mask)
-    self.set_unit(self, result._unit)
+    result = self ** arg
+    self._set_values(result._values, result._mask)
+    self.set_unit(result._unit)
     return self

Optionally, you may also want to update the docstring “Returns: … after the modulus operation.” to say “after the exponentiation operation.”

polymath/quaternion.py (1)

254-273: Consider simplifying scalar pnorm handling.

The handling of scalar vs array pnorm can be simplified using np.atleast_1d or by always treating it as an array.

-        # Scale by sqrt(2) to eliminate need to keep multiplying by 2
-        # Handle scalar pnorm case
-        if np.shape(pnorm) == ():
-            pnorm_array = np.array(pnorm)
-        else:
-            pnorm_array = pnorm
-        qvals = np.sqrt(2) / pnorm_array[..., np.newaxis] * pvals
+        # Scale by sqrt(2) to eliminate need to keep multiplying by 2
+        qvals = np.sqrt(2) / np.atleast_1d(pnorm)[..., np.newaxis] * pvals

Alternatively, np.asarray(pnorm) would also work since it handles both scalar and array inputs.

♻️ Duplicate comments (3)
polymath/vector3.py (1)

69-91: Vector3.from_scalars None-handling aligns with docs and denominator semantics

The new logic:

  • Treats all-None (x, y, z) by creating three Scalar(0.) components.
  • For mixed-None, converts non-None args to Scalars, broadcasts them to a common denominator via Qube.broadcast, and uses example_scalar.zero() to generate matching zero-valued replacements.

This matches the updated docstring and cleanly handles denominator shape inference.

Also applies to: 93-125

polymath/polynomial.py (1)

169-203: invert_line double-handles derivatives and applies the wrong transform

The coefficients a_inv and -b*a_inv are built as Scalars with derivatives, so result = Polynomial(Vector.from_scalars(a_inv, -b * a_inv)) already carries the correct derivatives via chain rule.

The subsequent block:

if recursive and self._derivs:
    for key, deriv in self._derivs.items():
        result.insert_deriv(key, deriv.invert_line(recursive=False))

overwrites those correct derivatives with deriv.invert_line(...), which is not the derivative of the inverse map; it is the inverse of the derivative polynomial, a different operation.

Recommendation: drop this loop and rely on the Scalar-based propagation:

-        result = Polynomial(Vector.from_scalars(a_inv, -b * a_inv))
-
-        # Handle derivatives if recursive
-        if recursive and self._derivs:
-            for key, deriv in self._derivs.items():
-                result.insert_deriv(key, deriv.invert_line(recursive=False))
-
-        return result
+        return Polynomial(Vector.from_scalars(a_inv, -b * a_inv))

This keeps invert_line mathematically correct and avoids conflicting derivative definitions.

tests/test_polynomial_basic.py (1)

96-106: invert_line derivative test doesn’t validate numerical behavior

This test only asserts that p_inv_with_deriv.d_dt exists and is a Polynomial, but not that its coefficients are correct. Given the complexity of invert_line’s derivative handling, it would be safer to assert expected coefficients explicitly (e.g., via a hand-derived chain-rule expression) and fix the inline comment to describe the actual polynomial (3x + 2).

Stronger assertions here would have caught the current derivative bug in invert_line.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 21

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
polymath/matrix.py (2)

34-58: Matrix.as_matrix ignores recursive for Vector inputs

Matrix.as_matrix(v, recursive=False) currently calls v.join_items([Matrix]) without using recursive, so derivatives on a Vector with drank == 1 will still be present on the resulting Matrix. New tests in tests/test_matrix_misc.py expect derivatives to be dropped when recursive=False.

To align behavior with the docstring and tests, consider:

     @staticmethod
     def as_matrix(arg, *, recursive=True):
@@
-        if isinstance(arg, Qube):
-
-            # Convert a Vector with drank=1 to a Matrix
-            if isinstance(arg, Vector) and arg._drank == 1:
-                return arg.join_items([Matrix])
-
-            arg = Matrix(arg._values, arg._mask, example=arg)
-            return arg if recursive else arg.wod
+        if isinstance(arg, Qube):
+
+            # Convert a Vector with drank=1 to a Matrix
+            if isinstance(arg, Vector) and arg._drank == 1:
+                result = arg.join_items([Matrix])
+                return result if recursive else result.wod
+
+            arg = Matrix(arg._values, arg._mask, example=arg)
+            return arg if recursive else arg.wod

177-232: from_scalars shape/size validation and recursive handling look correct

The updated from_scalars logic (using shape[0] * shape[1] and passing recursive through to reshape_numer) is consistent with Qube.reshape_numer semantics and the new tests. One small style tweak that addresses the Ruff hint would be:

-        return vector.reshape_numer(shape, list(classes) + [Matrix], recursive=recursive)
+        return vector.reshape_numer(shape, [*list(classes), Matrix], recursive=recursive)

Functionally, though, this block is solid as-is.

polymath/extensions/vector_ops.py (2)

148-169: Clarify axis validation contract for _zero_sized_result

_zero_sized_result assumes axis (int or sequence) indexes self._ndims * [slice(None)] directly, including negative indices. That’s safe as long as callers always run _check_axis (or equivalent) first, but this is not enforced here. It would help future maintainers if the docstring explicitly stated that axis must already be validated (e.g., via _check_axis), or, alternatively, if _zero_sized_result called _check_axis itself for the non-None case.


524-575: Consider adding return type hints and tightening error reporting in _cross_3x3 / _cross_2x2

The new private helpers are clear and nicely documented. Two small polish items you might consider:

  • Add return type annotations (-> np.ndarray) to _cross_3x3 and _cross_2x2 to help static analysis and IDEs.
  • The ValueError messages are fine, but if you ever wrap them, use raise ... from None to avoid noisy tracebacks from internal broadcasting/shape logic.

Both are non-functional, optional cleanups that align with the Ruff ANN202/TRY003 hints.

♻️ Duplicate comments (9)
polymath/matrix3.py (1)

837-837: Experimental methods appropriately marked.

These experimental __getstate__ and __setstate__ methods are correctly marked with pragma: no cover. The static analysis hint about missing return type annotations (ANN202) can be deferred since these are experimental and not part of the public API.

Also applies to: 879-879

polymath/extensions/math_ops.py (1)

380-383: Docstring correctly documents as_scalar() coercion for multiplication.

The docstring accurately reflects the implementation at line 398 which uses Qube._SCALAR_CLASS.as_scalar(arg) for non-Qube arguments.

tests/test_qube_unit.py (1)

277-315: The derivative conversion comments could be clearer.

At lines 304-307, the comments describe the derivative conversions. The test at line 305 expects (400000, 500000, 600000) for da_dt which has unit=Unit.CM/Unit.S. The internal values are in standard units (km/s), so converting to cm/s involves multiplying by 100000. This matches the expected values.

However, the comment "CM/S to M/S" at line 304 is slightly confusing since the unit is CM/S, not converting from CM/S to M/S. Consider clarifying that this is converting from standard units (km/s) to the declared unit (cm/s).

tests/test_polynomial_basic.py (1)

96-105: Invert-line derivative helper polynomial/comment are inconsistent with the linear polynomial

Here p_linear_with_deriv is Polynomial([3., 2.]) (3x + 2), but p_linear_deriv = Polynomial([1., 0.]) and the comment claim a derivative of 2. For 3x + 2, the derivative should be the constant 3 (e.g., Polynomial([3.]) under your coefficient convention).

Tests currently only check that a derivative Polynomial exists, so behavior is unaffected, but this mismatch is confusing. Consider updating either the polynomial or the comment (and ideally adding a small numeric check) so they describe the same derivative.

tests/test_qube_ext_item_ops.py (1)

491-529: Strengthen chain tests with numeric assertions

The chain coverage currently verifies shape, numer, and denom, but never checks the actual numeric result of the dot-product-like operation. That leaves the core math logic unverified.

Consider adding at least one small example where you assert c.values (or a slice) equals a hand-computed result, so changes to the chaining multiplication can’t silently pass as long as shapes stay the same.

polymath/extensions/pickler.py (1)

295-321: Simplify _validate_pickle_reference error handling and align with _validate_pickle_digits

The function still does an inner raise ValueError(...) inside the loop and then catches ValueError in the outer except just to raise a second ValueError based on original_references. That indirection isn’t buying much and makes the control flow harder to follow, especially now that you preserve the original input in the outer message.

You can keep behavior and satisfy Ruff’s TRY/B904 hints by removing the inner raise and letting invalid values fall through to the outer except, e.g.:

 def _validate_pickle_reference(references):
@@
-    try:
-        references = references[:2]
-        for reference in references[:2]:
-            if isinstance(reference, numbers.Real):
-                pass
-            elif reference not in {'smallest', 'largest', 'mean', 'median', 'logmean',
-                                   'fpzip'}:
-                raise ValueError(f'invalid pickle reference {reference!r}')
-
-    except (ValueError, IndexError, TypeError):
-        raise ValueError(f'invalid pickle reference {original_references!r}')
+    try:
+        references = references[:2]
+        for reference in references:
+            if isinstance(reference, numbers.Real):
+                continue
+            if reference not in {'smallest', 'largest', 'mean', 'median', 'logmean',
+                                 'fpzip'}:
+                raise TypeError  # trigger outer handler
+    except (IndexError, TypeError):
+        raise ValueError(
+            f'invalid pickle reference {original_references!r}'
+        ) from None

(or similar, e.g., by factoring the loop into a small helper). This keeps the external exception and message while simplifying the control flow.

tests/test_qube_coverage.py (1)

679-687: Prefer-builtin toggles now correctly guarded with try/finally

The Qube.prefer_builtins() state is saved and restored via try/finally in both the as_bool (Lines 679–687) and as_int (Lines 1420–1428) tests, so failures in the body no longer leak the global preference into later tests. This addresses the earlier global-state concern around these toggles.

Also applies to: 1420-1428

tests/test_qube_ext_vector_ops.py (1)

13-318: Unittest-centric style is fine; handle assertion-style linting via configuration instead

This module does a thorough job exercising vector ops (dot/norm/norm_sq/cross/outer/as_diagonal, reductions, and derivative propagation). Given it’s explicitly written as a unittest.TestCase with a large runTest, I’d keep the self.assert* / assertRaises style rather than rewriting to plain assert / pytest.raises just to satisfy assertion-style lint rules. If those rules are noisy here, it’s cleaner to relax or silence them for this file (or for unittest-based tests in general) instead of mixing styles.

Also applies to: 323-842

tests/test_qube_ext_pickler.py (1)

13-1219: Consider splitting this monolithic runTest into smaller, focused tests

This runTest method covers a huge number of distinct behaviors: per-instance vs default pickle digits, reference handling, float/int/bool encodings, fpzip paths, constant/items encodings, mask encodings (ALL_MASKED/CORNERS/ANTIMASKED/etc.), derivative round-trips, readonly flags, invalid encodings, and more. It’s great for coverage, but any failure will be hard to localize, and evolving pickler internals will make this method increasingly brittle.

You’d get more maintainable signal by splitting into multiple test methods or even multiple TestCase classes grouped by concern (e.g., configuration/digits, float encodings, mask encodings, fpzip paths, invalid encodings). The logic won’t change, but failures will be easier to diagnose and future contributors will find it easier to extend.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 18

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
polymath/extensions/vector_ops.py (1)

525-551: LGTM!

The rename to _cross_3x3 correctly signals this is an internal helper, and the updated docstring clearly documents its purpose.

For consistency with type-annotated codebases, consider adding return type:

-def _cross_3x3(a, b):
+def _cross_3x3(a, b) -> np.ndarray:
♻️ Duplicate comments (6)
polymath/polynomial.py (2)

813-813: Apply tuple unpacking for index construction (style consistency).

Per Ruff RUF005, use tuple unpacking instead of concatenation:

-                        root_mask[(k,) + mask_indices] = True
+                        root_mask[(k, *mask_indices)] = True

192-203: Remove incorrect derivative handling—Scalar arithmetic already propagates derivatives correctly.

The loop at lines 199-201 that calls deriv.invert_line(recursive=False) is mathematically incorrect and redundant:

  1. Automatic propagation: to_scalars(recursive=recursive) returns a and b with their derivatives attached, and the Scalar operations 1./a and -b * a_inv at lines 194-195 already apply the chain rule to propagate derivatives into the inverted coefficients.

  2. Incorrect transformation: Calling invert_line() on the derivative polynomial is not the correct derivative of the inverse. For the inverse transformation x = (y - b)/a, the derivative is dx/dt = (dy/dt · a - (y-b) · da/dt) / a², not invert_line(dy/dt).

  3. Overwrites correct result: The loop overwrites the automatically-propagated (correct) derivatives with an incorrect transformation.

Remove the derivative loop entirely:

     (a, b) = self.to_scalars(recursive=recursive)

     a_inv = 1. / a
     result = Polynomial(Vector.from_scalars(a_inv, -b * a_inv))

-    # Handle derivatives if recursive
-    # XXX Code Rabbit claims that this math is not correct - check it
-    if recursive and self._derivs:
-        for key, deriv in self._derivs.items():
-            result.insert_deriv(key, deriv.invert_line(recursive=False))
-
     return result

The recursive parameter in to_scalars already ensures derivatives are handled correctly through Scalar arithmetic.

tests/test_qube_ext_shrinker.py (1)

13-754: Break up the monolithic runTest and avoid line‑number‑based comments.

This single runTest mixes many independent behaviors (basic shrink/unshrink round‑trips, cache flags, all‑masked cases, broadcast paths, derivative propagation, internal _is_array/_is_scalar tricks). It’s very hard to localize a failure and some comments still refer to specific lines in shrinker.py (e.g., “lines 63–65”, “lines 173–174”), which will go stale when that file changes.

Consider:

  • Splitting this into focused test methods (e.g., test_basic_shrink, test_unshrink_cache_flags, test_broadcasting_paths, test_all_masked, test_derivatives_and_nested_derivatives), and
  • Rephrasing implementation‑detail comments to describe semantic conditions (“antimask has more dimensions than self and triggers broadcast_to”, “all selected mask is True so shrink returns masked_single”) and asserting those behaviors directly (shapes, masks, readonly flags), instead of relying on line numbers for coverage mapping.

This will keep the coverage intent but make the suite more maintainable as shrinker.py evolves.

tests/test_qube_coverage.py (1)

93-139: Tighten private‑helper tests that currently “may or may not raise”.

Blocks like:

  • _as_mask(object(), opstr='test'),
  • _as_mask([1,2,3], opstr='test') # Not boolean,
  • _suitable_mask([True, False], shape=(2,), opstr='test'),
  • _suitable_numer('invalid', opstr='test'),

are wrapped in try/except with comments such as “may or may not raise” and no assertion beyond pass in the except. This means they don’t fail even if behavior flips (e.g., from raising to silently accepting bad input).

Where the intent is to verify an error path, assert the specific exception with with self.assertRaises(...). If behavior is genuinely undefined and you only want coverage, consider deleting the block or documenting and asserting the behavior you actually support instead of “may or may not raise”.

Also applies to: 149-172

tests/test_qube_ext_pickler.py (2)

679-693: Duplicate test block - consider removing.

This block (lines 679-693) is identical to lines 422-439, testing the same _PICKLE_DEBUG functionality. A third similar test exists at lines 1060-1078 using direct module access.

Consider keeping only one comprehensive test for _PICKLE_DEBUG functionality and removing the duplicates to improve maintainability.


15-17: Consider splitting the monolithic test method in a follow-up.

The single runTest method spans ~1200 lines and mixes many concerns. While this provides good coverage for the PR's goals, splitting into focused test methods (e.g., test_pickle_digits_config, test_float_encodings, test_mask_encodings, test_error_paths) would improve failure diagnostics and maintainability.

This is a recommended refactor for a follow-up, not a blocker for this PR.

Comment on lines +61 to +65
else: # pragma: no cover
# This is unreachable because if arg._shape is (), then the mask is boolean
# and either mask=False (line 50 hits) or mask=True (line 54 hits).
raise RuntimeError('This should be unreachable')
# obj = arg
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Good fix for the unreachable branch.

This addresses the defensive coding concern appropriately. However, the commented-out # obj = arg on line 65 is now dead code and should be removed.

         else:  # pragma: no cover
             # This is unreachable because if arg._shape is (), then the mask is boolean
             # and either mask=False (line 50 hits) or mask=True (line 54 hits).
             raise RuntimeError('This should be unreachable')
-            # obj = arg
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
else: # pragma: no cover
# This is unreachable because if arg._shape is (), then the mask is boolean
# and either mask=False (line 50 hits) or mask=True (line 54 hits).
raise RuntimeError('This should be unreachable')
# obj = arg
else: # pragma: no cover
# This is unreachable because if arg._shape is (), then the mask is boolean
# and either mask=False (line 50 hits) or mask=True (line 54 hits).
raise RuntimeError('This should be unreachable')
🧰 Tools
🪛 Ruff (0.14.8)

64-64: Avoid specifying long messages outside the exception class

(TRY003)

🤖 Prompt for AI Agents
In polymath/extensions/vector_ops.py around lines 61 to 65, remove the dead
commented-out line "# obj = arg" (line 65) since the unreachable branch already
raises RuntimeError and the commented assignment is no longer needed; simply
delete that comment so the unreachable branch contains only the RuntimeError
raise and no leftover dead code.

Comment on lines 137 to 141
try:
_ = selections[i]
except IndexError:
raise IndexError(f'axis is out of range ({-arg._rank},{arg._rank}) in '
raise IndexError(f'axis is out of range ({-arg._ndims},{arg._ndims}) in '
f'{type(arg)}.{op}: {i}')
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Chain the exception to preserve traceback context.

When re-raising an exception within an except clause, use exception chaining to preserve the original traceback.

         try:
             _ = selections[i]
         except IndexError:
-            raise IndexError(f'axis is out of range ({-arg._ndims},{arg._ndims}) in '
-                             f'{type(arg)}.{op}: {i}')
+            raise IndexError(f'axis is out of range ({-arg._ndims},{arg._ndims}) in '
+                             f'{type(arg)}.{op}: {i}') from None
🧰 Tools
🪛 Ruff (0.14.8)

140-141: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


140-141: Avoid specifying long messages outside the exception class

(TRY003)

🤖 Prompt for AI Agents
In polymath/extensions/vector_ops.py around lines 137 to 141, the IndexError is
re-raised without chaining which loses the original traceback; change the except
block to capture the caught exception (e.g. except IndexError as e:) and
re-raise the new IndexError using exception chaining (raise IndexError(f'axis is
out of range ({-arg._ndims},{arg._ndims}) in {type(arg)}.{op}: {i}') from e) so
the original traceback is preserved.

Comment on lines +254 to 288
self.require_writeable()
arg = Polynomial.as_polynomial(arg)
# Ensure both have compatible orders
max_order = max(self.order, arg.order)
if self.order < max_order:
# Pad self in-place by resizing _values
new_values = np.zeros(self._shape + (max_order+1,))
new_values[..., -self.order-1:] = self._values
# Use a temporary Polynomial to compute invariants from new_values
# This ensures we use the same logic as the constructor
temp = Polynomial(new_values, self._mask, example=self)
# Copy the shape metadata back into self
self._values = temp._values
self._shape = temp._shape
self._ndims = temp._ndims
self._item = temp._item
self._numer = temp._numer
self._denom = temp._denom
self._isize = temp._isize
self._nsize = temp._nsize
self._dsize = temp._dsize
self._is_array = temp._is_array
self._is_scalar = temp._is_scalar
if arg.order < max_order:
arg = arg.at_least_order(max_order)
# Perform addition in-place
self._values += arg._values
self._mask = Qube.or_(self._mask, arg._mask)
self._unit = self._unit or arg._unit
# Handle derivatives
if self._derivs or arg._derivs:
new_derivs = self._add_derivs(self, arg)
self.insert_derivs(new_derivs)
self._cache.clear()
return self
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Consider factoring out the state-copying pattern into a helper method.

The in-place padding logic (lines 258-276) creates a temporary Polynomial to recompute invariants and then manually copies 12+ attributes back into self. This pattern:

  • Is duplicated in __isub__ (lines 332-350)
  • Is fragile: if Qube or Vector adds new internal state fields, both methods must be updated
  • Could be centralized

Consider adding a helper method like _replace_values(self, new_values) that encapsulates this logic:

def _replace_values(self, new_values):
    """Replace self._values and recompute all derived shape metadata."""
    temp = Polynomial(new_values, self._mask, example=self)
    # Copy shape metadata from temp
    for attr in ('_values', '_shape', '_ndims', '_item', '_numer', '_denom',
                 '_isize', '_nsize', '_dsize', '_is_array', '_is_scalar'):
        setattr(self, attr, getattr(temp, attr))

Then both __iadd__ and __isub__ can call self._replace_values(new_values), reducing duplication and making future maintenance easier.

🧰 Tools
🪛 Ruff (0.14.8)

260-260: Consider (*self._shape, max_order + 1) instead of concatenation

Replace with (*self._shape, max_order + 1)

(RUF005)

🤖 Prompt for AI Agents
polymath/polynomial.py lines 254-288: the in-place padding block creates a
temporary Polynomial and then manually copies a long list of internal attributes
back into self; extract this into a helper method to avoid duplication and
fragility. Add a method e.g. _replace_values(self, new_values) which constructs
temp = Polynomial(new_values, self._mask, example=self) and then copies the
derived attributes from temp to self by iterating over the attribute names
('_values','_shape','_ndims','_item','_numer','_denom','_isize','_nsize','_dsize','_is_array','_is_scalar')
using setattr/getattr, then replace the manual copy block here with a call to
self._replace_values(new_values) and make the same change in __isub__ so both
use the new helper.

Comment on lines +1127 to +1133
class NoDerivsQube(Qube):
_DERIVS_OK = False
try:
_ = NoDerivsQube(1., derivs={'t': Scalar(0.1)})
self.fail("Expected ValueError for disallowed derivatives")
except ValueError:
pass
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Deduplicate NoDerivsQube helper class.

NoDerivsQube(Qube) is defined twice with the same body (once for the “derivatives disallowed” __init__ test and again near the end for as_this_type commentary). This shadows the earlier definition without adding behavior.

Define NoDerivsQube once (either at module scope or above the first use) and reuse it in both tests. This avoids confusion about which version is active and keeps helper intent clearer.

Also applies to: 1462-1464

🤖 Prompt for AI Agents
In tests/test_qube_coverage.py around lines 1127-1133 (and similarly at
1462-1464), the helper class NoDerivsQube is duplicated; remove the second
definition and define NoDerivsQube once (either at module scope near the top of
the test file or above the first test that uses it) and update both test sites
to import/ reference that single definition so both tests reuse the same helper
class without shadowing.

Comment on lines +384 to +390
# Check that FLOAT encoding is present
vals_encoding = state['VALS_ENCODING']
# May or may not have FLOAT depending on compression method
# Check encoding structure (has_float variable kept for potential future use)
_ = any(item[0] == 'FLOAT' for item in vals_encoding
if isinstance(item, tuple))

Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Remove dead code or convert to assertion.

The result is computed but discarded to _. Either remove this block or convert it to an actual assertion if the check is meaningful:

-        # Check encoding structure (has_float variable kept for potential future use)
-        _ = any(item[0] == 'FLOAT' for item in vals_encoding
-                if isinstance(item, tuple))
+        # Verify FLOAT encoding is present for floating-point arrays
+        has_float = any(item[0] == 'FLOAT' for item in vals_encoding
+                        if isinstance(item, tuple))
+        self.assertTrue(has_float, "Expected FLOAT encoding for float array")

Alternatively, if the encoding type varies based on array size (literal vs. FLOAT), simply remove the dead code.

🤖 Prompt for AI Agents
In tests/test_qube_ext_pickler.py around lines 384 to 390, the code computes a
boolean into a throwaway variable "_" when checking for a 'FLOAT' encoding in
vals_encoding; either remove this dead computation entirely if the presence of
FLOAT is expected to vary, or replace it with an assert (e.g., assert any(...))
so the test actually fails when the encoding is wrong — update the test to
remove the unused `_ =` assignment and either delete the whole block or convert
it to a meaningful assertion depending on the intended behavior.

Comment on lines +52 to +56
# Test with top parameter
s9 = Scalar([1, 2, 3, 4, 5])
s10 = s9.int(top=3, remask=True)
self.assertTrue(s10.mask[3] or s10.mask[4])

Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Strengthen int(top=3, remask=True) mask assertion

Right now the test only checks s10.mask[3] or s10.mask[4], which would still pass if only one of the out-of-range entries were masked or if earlier entries were accidentally masked.

If the contract is that all entries with value ≥ top are masked, it would be better to assert that explicitly, e.g.:

-        s10 = s9.int(top=3, remask=True)
-        self.assertTrue(s10.mask[3] or s10.mask[4])
+        s10 = s9.int(top=3, remask=True)
+        # Entries >= top (indices 3 and above) should be masked; earlier ones unmasked.
+        mask = np.asarray(s10.mask)
+        self.assertTrue(np.all(mask[3:]))
+        self.assertTrue(np.all(~mask[:3]))

(adjust if the implementation intentionally uses a scalar mask rather than an array).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Test with top parameter
s9 = Scalar([1, 2, 3, 4, 5])
s10 = s9.int(top=3, remask=True)
self.assertTrue(s10.mask[3] or s10.mask[4])
# Test with top parameter
s9 = Scalar([1, 2, 3, 4, 5])
s10 = s9.int(top=3, remask=True)
# Entries >= top (indices 3 and above) should be masked; earlier ones unmasked.
mask = np.asarray(s10.mask)
self.assertTrue(np.all(mask[3:]))
self.assertTrue(np.all(~mask[:3]))
🧰 Tools
🪛 Ruff (0.14.8)

55-55: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)

🤖 Prompt for AI Agents
In tests/test_scalar_comprehensive.py around lines 52 to 56, the assertion for
s10 = s9.int(top=3, remask=True) only checks s10.mask[3] or s10.mask[4], which
is too weak; change the test to explicitly assert that all entries with value >=
top are masked (e.g., assert s10.mask[3] and s10.mask[4] or, if mask is scalar,
assert appropriate aggregated condition), so the test fails if any out-of-range
element is not masked or if earlier entries are wrongly masked.

Comment on lines +25 to +1230
class Test_Scalar_Coverage(unittest.TestCase):

def runTest(self):

np.random.seed(54321)

##################################################################################
# Test _minval and _maxval edge cases
##################################################################################
# Test invalid dtype
try:
dtype = np.dtype('U') # Unicode string dtype
_ = Scalar._minval(dtype)
except ValueError:
pass # Expected

try:
dtype = np.dtype('U')
_ = Scalar._maxval(dtype)
except ValueError:
pass # Expected

# Test all dtype kinds
for kind in ['f', 'u', 'i']:
dtype = np.dtype(kind + '8')
min_val = Scalar._minval(dtype)
max_val = Scalar._maxval(dtype)
self.assertIsNotNone(min_val)
self.assertIsNotNone(max_val)

# Test boolean dtype separately
dtype = np.dtype('bool')
min_val = Scalar._minval(dtype)
max_val = Scalar._maxval(dtype)
self.assertIsNotNone(min_val)
self.assertIsNotNone(max_val)

##################################################################################
# Test as_scalar edge cases
##################################################################################
# Test with Boolean
b = Boolean(True)
s = Scalar.as_scalar(b)
self.assertEqual(s, 1)

# Test with Qube that's not Scalar
# Vector has nrank=1, so converting to Scalar (nrank=0) will fail
# This tests the error path
try:
v = Vector([1., 2., 3.])
s = Scalar.as_scalar(v)
# If it succeeds, verify it's a Scalar
self.assertEqual(type(s), Scalar)
except ValueError:
pass # Expected - Vector can't be converted to Scalar due to rank mismatch

# Test with Unit
s = Scalar.as_scalar(Unit.KM)
self.assertIsNotNone(s.unit_)

# Test recursive=False
a = Scalar([1., 2., 3.])
a.insert_deriv('t', Scalar([0.1, 0.2, 0.3]))
s = Scalar.as_scalar(a, recursive=False)
self.assertFalse(hasattr(s, 'd_dt'))

##################################################################################
# Test to_scalar error case
##################################################################################
# Test index out of range
a = Scalar(1.)
self.assertRaises(ValueError, a.to_scalar, 1)

# Test recursive=False
a = Scalar(1.)
a.insert_deriv('t', Scalar(0.1))
s = a.to_scalar(0, recursive=False)
self.assertFalse(hasattr(s, 'd_dt'))

##################################################################################
# Test as_index_and_mask error cases
##################################################################################
# Test floating-point indexing
a = Scalar([1.5, 2.5, 3.5])
self.assertRaises(IndexError, a.as_index_and_mask)

# Test with denominator
try:
a = Vector(np.arange(6).reshape(2, 3), drank=1)
_ = a.as_index_and_mask()
except ValueError:
pass # Expected

# Test purge=True with all masked
a = Scalar([1, 2, 3], mask=True)
idx, mask = a.as_index_and_mask(purge=True)
self.assertEqual(len(idx), 0)

# Test purge=True with partially masked
a = Scalar([1, 2, 3])
a = a.mask_where_eq(2)
idx, mask = a.as_index_and_mask(purge=True)
self.assertEqual(len(idx), 2)

# Test masked=None with all masked
a = Scalar([1, 2, 3], mask=True)
idx, mask = a.as_index_and_mask(masked=999)
self.assertTrue(np.all(idx == 999))

# Test masked=None with partially masked
a = Scalar([1, 2, 3])
a = a.mask_where_eq(2)
idx, mask = a.as_index_and_mask(masked=999)
self.assertEqual(idx[1], 999)

##################################################################################
# Test int() error cases
##################################################################################
# Test with denominator
try:
a = Vector(np.arange(6).reshape(2, 3), drank=1)
_ = a.int()
except ValueError:
pass # Expected

# Test with top parameter and shift
a = Scalar([1, 2, 3, 4, 5])
b = a.int(top=3, shift=True, clip=False)
# shift=True means shift values equal to top down by 1
# So value 3 at index 2 should become 2, value 4 at index 3 should become 3, etc.
# Actually, the logic shifts values equal to top, so if top=3, values of 3 become 2
# Let's just verify the operation completes
self.assertEqual(len(b), 5)

# Test with remask and clip
a = Scalar([1, 2, 3, 4, 5])
b = a.int(top=3, remask=True, clip=False)
self.assertTrue(b.mask[3] or b.mask[4])

# Test with clip=True
a = Scalar([1, 2, 3, 4, 5])
b = a.int(top=3, clip=True)
self.assertTrue(np.all(b.values <= 2))

# Test with remask and no top
a = Scalar([-1, 0, 1, 2, 3])
b = a.int(remask=True, clip=False)
self.assertTrue(b.mask[0])

# Test builtins
a = Scalar(5.7)
with prefer_builtins(True):
b = a.int()
self.assertIsInstance(b, int)

##################################################################################
# Test frac() error case
##################################################################################
# Test with denominator
# frac() is a Scalar method, so test with Scalar that has denominator
# Actually, Scalar can't have denominator, so this test is hard to do
# Let's just test that frac() works normally
a = Scalar([1.5, 2.5, 3.5])
b = a.frac()
self.assertTrue(np.allclose(b.values, [0.5, 0.5, 0.5]))

# Test with derivatives
a = Scalar([1.5, 2.5, 3.5])
a.insert_deriv('t', Scalar([0.1, 0.2, 0.3]))
b = a.frac(recursive=True)
self.assertTrue(hasattr(b, 'd_dt'))

##################################################################################
# Test sin() error case
##################################################################################
# Test with denominator
# sin() is a Scalar method, and Scalar can't have denominator
# So this error case is hard to test directly
# Let's just test that sin() works normally
a = Scalar([0., np.pi/2, np.pi], unit=Unit.RAD)
b = a.sin()
self.assertTrue(np.allclose(b.values, [0., 1., 0.], atol=1e-10))

##################################################################################
# Test cos() error case
##################################################################################
# Test with denominator
# cos() is a Scalar method, and Scalar can't have denominator
# Let's just test that cos() works normally
a = Scalar([0., np.pi/2, np.pi], unit=Unit.RAD)
b = a.cos()
self.assertTrue(np.allclose(b.values, [1., 0., -1.], atol=1e-10))

##################################################################################
# Test tan() error case
##################################################################################
# Test with denominator
# tan() is a Scalar method, and Scalar can't have denominator
# Let's just test that tan() works normally
a = Scalar([0., np.pi/4], unit=Unit.RAD)
b = a.tan()
self.assertTrue(np.allclose(b.values, [0., 1.], atol=1e-10))

##################################################################################
# Test arcsin() error cases
##################################################################################
# Test with denominator
# arcsin() is a Scalar method, and Scalar can't have denominator
# Let's just test that arcsin() works normally
a = Scalar([0., 0.5, 1.])
b = a.arcsin()
self.assertTrue(np.allclose(b.values, [0., np.arcsin(0.5), np.pi/2], atol=1e-10))

# Test with check=False and invalid value
a = Scalar(2.) # Outside [-1, 1]
with warnings.catch_warnings():
warnings.filterwarnings('error')
try:
_ = a.arcsin(check=False)
except (ValueError, RuntimeWarning):
pass # Expected

# Test with check=True and invalid values
a = Scalar([-2., 0., 2.])
b = a.arcsin(check=True)
self.assertTrue(b.mask[0] or b.mask[2])

##################################################################################
# Test arccos() error cases
##################################################################################
# Test with denominator
# arccos() is a Scalar method, and Scalar can't have denominator
# Let's just test that arccos() works normally
a = Scalar([1., 0.5, 0.])
b = a.arccos()
self.assertTrue(np.allclose(b.values, [0., np.arccos(0.5), np.pi/2], atol=1e-10))

# Test with check=False and invalid value
a = Scalar(2.) # Outside [-1, 1]
with warnings.catch_warnings():
warnings.filterwarnings('error')
try:
_ = a.arccos(check=False)
except (ValueError, RuntimeWarning):
pass # Expected

# Test with check=True and invalid values
a = Scalar([-2., 0., 2.])
b = a.arccos(check=True)
self.assertTrue(b.mask[0] or b.mask[2])

##################################################################################
# Test arctan() error case
##################################################################################
# Test with denominator
# arctan() is a Scalar method, and Scalar can't have denominator
# Let's just test that arctan() works normally
a = Scalar([0., 1., -1.])
b = a.arctan()
self.assertTrue(np.allclose(b.values, [0., np.pi/4, -np.pi/4], atol=1e-10))

##################################################################################
# Test arctan2() error case
##################################################################################
# Test with denominator
# arctan2() requires both arguments to be Scalars without denominators
# Let's test the normal case
a = Scalar(1.)
b = Scalar(1.)
c = a.arctan2(b)
self.assertAlmostEqual(c, np.pi/4, places=10)

##################################################################################
# Test sqrt() error cases
##################################################################################
# Test with denominator
# sqrt() is a Scalar method, and Scalar can't have denominator
# Let's just test that sqrt() works normally
a = Scalar([1., 4., 9.])
b = a.sqrt()
self.assertTrue(np.allclose(b.values, [1., 2., 3.]))

# Test with check=False and negative value
a = Scalar(-1.)
with warnings.catch_warnings():
warnings.filterwarnings('error')
try:
_ = a.sqrt(check=False)
except (ValueError, RuntimeWarning):
pass # Expected

##################################################################################
# Test log() error cases
##################################################################################
# Test with denominator
# log() is a Scalar method, and Scalar can't have denominator
# Let's just test that log() works normally
a = Scalar([1., np.e, np.e**2])
b = a.log()
self.assertTrue(np.allclose(b.values, [0., 1., 2.], atol=1e-10))

# Test with check=False and non-positive value
a = Scalar(0.)
with warnings.catch_warnings():
warnings.filterwarnings('error')
try:
_ = a.log(check=False)
except (ValueError, RuntimeWarning):
pass # Expected

##################################################################################
# Test exp() error cases
##################################################################################
# Test with denominator
# exp() is a Scalar method, and Scalar can't have denominator
# Let's just test that exp() works normally
a = Scalar([0., 1., 2.])
b = a.exp()
self.assertTrue(np.allclose(b.values, [1., np.e, np.e**2], atol=1e-10))

# Test with check=False and overflow
a = Scalar(1000.) # Very large value
with warnings.catch_warnings():
warnings.filterwarnings('error')
try:
_ = a.exp(check=False)
except (ValueError, TypeError, RuntimeWarning):
pass # May overflow and raise RuntimeWarning

# Test with check=True and overflow
a = Scalar(1000.)
b = a.exp(check=True)
# Should mask overflow values

##################################################################################
# Test sign() edge cases
##################################################################################
# Test with zeros=False
a = Scalar([-1., 0., 1.])
b = a.sign(zeros=False)
self.assertEqual(b[1], 1) # Zero should become 1

# Test builtins
a = Scalar(1.)
with prefer_builtins(True):
b = a.sign()
# sign() returns the sign, which for float 1.0 is 1.0 (float), not int
# But if it's an integer Scalar, it might return int
a_int = Scalar(1) # Integer
b_int = a_int.sign()
# The result type depends on the input type
self.assertIsInstance(b, (int, float))
self.assertIsInstance(b_int, int)
self.assertEqual(b_int, 1)

##################################################################################
# Test max() error case
##################################################################################
# Test with denominator
# max() is a Scalar method, and Scalar can't have denominator
# Let's just test that max() works normally
a = Scalar([1., 3., 2.])
b = a.max()
self.assertEqual(b, 3.)

# Test with all masked
a = Scalar([1., 2., 3.], mask=True)
b = a.max()
self.assertTrue(b.mask)

# Test with partially masked
a = Scalar([1., 2., 3.])
a = a.mask_where_eq(2.)
b = a.max()
self.assertEqual(b, 3.)

# Test builtins
a = Scalar([1., 2., 3.])
with prefer_builtins(True):
b = a.max()
self.assertIsInstance(b, (int, float))

##################################################################################
# Test min() error case
##################################################################################
# Test with denominator
# min() is a Scalar method, and Scalar can't have denominator
# Let's just test that min() works normally
a = Scalar([3., 1., 2.])
b = a.min()
self.assertEqual(b, 1.)

# Test with all masked
a = Scalar([1., 2., 3.], mask=True)
b = a.min()
self.assertTrue(b.mask)

# Test with partially masked
a = Scalar([1., 2., 3.])
a = a.mask_where_eq(2.)
b = a.min()
self.assertEqual(b, 1.)

# Test builtins
a = Scalar([1., 2., 3.])
with prefer_builtins(True):
b = a.min()
self.assertIsInstance(b, (int, float))

##################################################################################
# Test argmax() error cases
##################################################################################
# Test with denominator
# argmax() is a Scalar method, and Scalar can't have denominator
# Let's just test that argmax() works normally
a = Scalar([1., 3., 2.])
b = a.argmax()
self.assertEqual(b, 1) # Index of max value

# Test with shape ()
a = Scalar(1.)
self.assertRaises(ValueError, a.argmax)

# Test with all masked
a = Scalar([1., 2., 3.], mask=True)
b = a.argmax()
self.assertTrue(b.mask)

# Test with partially masked
a = Scalar([1., 2., 3.])
a = a.mask_where_eq(2.)
b = a.argmax()
# Should return index of max unmasked value

# Test builtins
a = Scalar([1., 2., 3.])
with prefer_builtins(True):
b = a.argmax()
self.assertIsInstance(b, int)

##################################################################################
# Test argmin() error cases
##################################################################################
# Test with denominator
# argmin() is a Scalar method, and Scalar can't have denominator
# Let's just test that argmin() works normally
a = Scalar([3., 1., 2.])
b = a.argmin()
self.assertEqual(b, 1) # Index of min value

# Test with shape ()
a = Scalar(1.)
self.assertRaises(ValueError, a.argmin)

# Test with all masked
a = Scalar([1., 2., 3.], mask=True)
b = a.argmin()
self.assertTrue(b.mask)

# Test with partially masked
a = Scalar([1., 2., 3.])
a = a.mask_where_eq(2.)
b = a.argmin()
# Should return index of min unmasked value

# Test builtins
a = Scalar([1., 2., 3.])
with prefer_builtins(True):
b = a.argmin()
self.assertIsInstance(b, int)

##################################################################################
# Test maximum() error cases
##################################################################################
# Test missing arguments
self.assertRaises(ValueError, Scalar.maximum)

# Test with denominator
# maximum() is a Scalar static method, and Scalar can't have denominator
# Let's test the normal case
a = Scalar([1., 3., 2.])
b = Scalar([2., 1., 4.])
c = Scalar.maximum(a, b)
self.assertTrue(np.allclose(c.values, [2., 3., 4.]))

# Test with single argument
a = Scalar([1., 2., 3.])
b = Scalar.maximum(a)
self.assertTrue(np.allclose(b.values, a.values))

# Test with mixed int/float
a = Scalar([1, 2, 3])
b = Scalar([1., 2., 3.])
c = Scalar.maximum(a, b)
self.assertTrue(c.is_float())

##################################################################################
# Test minimum() error cases
##################################################################################
# Test missing arguments
self.assertRaises(ValueError, Scalar.minimum)

# Test with denominator
# minimum() is a Scalar static method, and Scalar can't have denominator
# Let's test the normal case
a = Scalar([1., 3., 2.])
b = Scalar([2., 1., 4.])
c = Scalar.minimum(a, b)
self.assertTrue(np.allclose(c.values, [1., 1., 2.]))

# Test with single argument
a = Scalar([1., 2., 3.])
b = Scalar.minimum(a)
self.assertTrue(np.allclose(b.values, a.values))

# Test with mixed int/float
a = Scalar([1, 2, 3])
b = Scalar([1., 2., 3.])
c = Scalar.minimum(a, b)
self.assertTrue(c.is_float())

##################################################################################
# Test median() error case
##################################################################################
# Test with denominator
# median() is a Scalar method, and Scalar can't have denominator
# Let's just test that median() works normally
a = Scalar([1., 3., 2., 4., 5.])
b = a.median()
self.assertEqual(b, 3.)

# Test with all masked
a = Scalar([1., 2., 3.], mask=True)
b = a.median()
self.assertTrue(b.mask)

# Test with axis=None and masked
a = Scalar([1., 2., 3., 4., 5.])
a = a.mask_where_eq(3.)
b = a.median(axis=None)
# Should compute median of unmasked values

# Test with axis and masked
a = Scalar(np.arange(24).reshape(2, 3, 4))
a = a.mask_where_eq(5.)
b = a.median(axis=0)
# Should compute median along axis 0

# Test builtins
a = Scalar([1., 2., 3., 4., 5.])
with prefer_builtins(True):
b = a.median()
self.assertIsInstance(b, float)

##################################################################################
# Test sort() error case
##################################################################################
# Test with denominator
# sort() is a Scalar method, and Scalar can't have denominator
# Let's just test that sort() works normally
a = Scalar([3., 1., 2.])
b = a.sort()
self.assertTrue(np.allclose(b.values, [1., 2., 3.]))

# Test with masked values
a = Scalar([3., 1., 2.])
a = a.mask_where_eq(2.)
b = a.sort()
# Masked values should appear at end

##################################################################################
# Test reciprocal() error cases
##################################################################################
# Test with denominator
# reciprocal() is a Scalar method, and Scalar can't have denominator
# The error check is for self._rank, not self._drank
# Let's test the normal case
a = Scalar([1., 2., 4.])
b = a.reciprocal()
self.assertTrue(np.allclose(b.values, [1., 0.5, 0.25]))

# Test with nozeros=True and zero
a = Scalar([1., 0., 2.])
with warnings.catch_warnings():
warnings.filterwarnings('error')
try:
_ = a.reciprocal(nozeros=True)
except ValueError:
pass # Expected

# Test with nozeros=False and zero
a = Scalar([1., 0., 2.])
b = a.reciprocal(nozeros=False)
self.assertTrue(b.mask[1]) # Zero should be masked

##################################################################################
# Test __pow__ error cases
##################################################################################
# Test with denominator
# __pow__ checks for denominator using _disallow_denom
# Scalar can't have denominator, so this is hard to test
# Let's test the normal case
a = Scalar([2., 3., 4.])
b = a ** 2
self.assertTrue(np.allclose(b.values, [4., 9., 16.]))

# Test with array exponent
a = Scalar([2., 3., 4.])
b = Scalar([1., 2.]) # Different shape
try:
_ = a ** b
except ValueError:
pass # Expected

# Test with unit and array exponent
a = Scalar([2., 3., 4.], unit=Unit.KM)
b = Scalar([1., 2.]) # Array exponent
try:
_ = a ** b
except ValueError:
pass # Expected

# Test with masked result
a = Scalar(0.)
b = Scalar(-1.)
try:
c = a ** b # 0 ** -1 is undefined
except (ValueError, ZeroDivisionError):
pass # May raise or mask

# Test with non-Real exponent
a = Scalar([2., 3., 4.])
try:
_ = a ** "invalid"
except (TypeError, ValueError):
pass # Expected

##################################################################################
# Test __le__, __lt__, __ge__, __gt__ with denominators
##################################################################################
# Test with denominators
try:
a = Scalar(1.)
b = Vector(np.arange(6).reshape(2, 3), drank=1)
_ = a <= b
except ValueError:
pass # Expected

try:
a = Scalar(1.)
b = Vector(np.arange(6).reshape(2, 3), drank=1)
_ = a < b
except ValueError:
pass # Expected

try:
a = Scalar(1.)
b = Vector(np.arange(6).reshape(2, 3), drank=1)
_ = a >= b
except ValueError:
pass # Expected

try:
a = Scalar(1.)
b = Vector(np.arange(6).reshape(2, 3), drank=1)
_ = a > b
except ValueError:
pass # Expected

# Test builtins
a = Scalar(1.)
b = Scalar(2.)
with prefer_builtins(True):
c = a <= b
self.assertIsInstance(c, bool)
c = a < b
self.assertIsInstance(c, bool)
c = a >= b
self.assertIsInstance(c, bool)
c = a > b
self.assertIsInstance(c, bool)

##################################################################################
# Test __round__
##################################################################################
a = Scalar(1.234567)
b = round(a, 2)
self.assertAlmostEqual(b, 1.23, places=2)

##################################################################################
# Test __abs__ with derivatives
##################################################################################
a = Scalar([-1., 2., -3.])
a.insert_deriv('t', Scalar([-0.1, 0.2, -0.3]))
b = abs(a)
self.assertTrue(hasattr(b, 'd_dt'))
# Derivatives should be multiplied by sign

##################################################################################
# Test _power_0 with derivatives
##################################################################################
a = Scalar([1., 2., 3.])
a.insert_deriv('t', Scalar([0.1, 0.2, 0.3]))
b = a._power_0(recursive=True)
self.assertTrue(hasattr(b, 'd_dt'))
# Derivatives should be zeros

##################################################################################
# Test _power_1
##################################################################################
a = Scalar([1., 2., 3.])
a.insert_deriv('t', Scalar([0.1, 0.2, 0.3]))
b = a._power_1(recursive=True)
self.assertTrue(hasattr(b, 'd_dt'))
b = a._power_1(recursive=False)
self.assertFalse(hasattr(b, 'd_dt'))

##################################################################################
# Test _power_2, _power_3, _power_4
##################################################################################
a = Scalar([1., 2., 3.])
a.insert_deriv('t', Scalar([0.1, 0.2, 0.3]))
b = a._power_2(recursive=True)
self.assertTrue(hasattr(b, 'd_dt'))
self.assertTrue(np.allclose(b.values, [1., 4., 9.]))

b = a._power_3(recursive=True)
self.assertTrue(hasattr(b, 'd_dt'))
self.assertTrue(np.allclose(b.values, [1., 8., 27.]))

b = a._power_4(recursive=True)
self.assertTrue(hasattr(b, 'd_dt'))
self.assertTrue(np.allclose(b.values, [1., 16., 81.]))

##################################################################################
# Test _power_neg_1, _power_half, _power_neg_half
##################################################################################
a = Scalar([1., 2., 4.])
a.insert_deriv('t', Scalar([0.1, 0.2, 0.3]))
b = a._power_neg_1(recursive=True)
self.assertTrue(hasattr(b, 'd_dt'))
self.assertTrue(np.allclose(b.values, [1., 0.5, 0.25]))

b = a._power_half(recursive=True)
self.assertTrue(hasattr(b, 'd_dt'))
self.assertTrue(np.allclose(b.values, [1., np.sqrt(2.), 2.]))

b = a._power_neg_half(recursive=True)
self.assertTrue(hasattr(b, 'd_dt'))
self.assertTrue(np.allclose(b.values, [1., 1./np.sqrt(2.), 0.5]))

##################################################################################
# Test __pow__ with easy powers
##################################################################################
a = Scalar([1., 2., 3.])
# Test power 0
b = a ** 0
self.assertTrue(np.allclose(b.values, [1., 1., 1.]))

# Test power 1
b = a ** 1
self.assertTrue(np.allclose(b.values, [1., 2., 3.]))

# Test power 2
b = a ** 2
self.assertTrue(np.allclose(b.values, [1., 4., 9.]))

# Test power 3
b = a ** 3
self.assertTrue(np.allclose(b.values, [1., 8., 27.]))

# Test power 4
b = a ** 4
self.assertTrue(np.allclose(b.values, [1., 16., 81.]))

# Test power -1
b = a ** -1
self.assertTrue(np.allclose(b.values, [1., 0.5, 1./3.]))

# Test power 0.5
b = a ** 0.5
self.assertTrue(np.allclose(b.values, [1., np.sqrt(2.), np.sqrt(3.)]))

# Test power -0.5
b = a ** -0.5
self.assertTrue(np.allclose(b.values, [1., 1./np.sqrt(2.), 1./np.sqrt(3.)]))

# Test with integer exponent that needs conversion
a = Scalar([1, 2, 3]) # Integer
b = Scalar(-1) # Negative integer exponent
c = a ** b
self.assertTrue(c.is_float()) # Should convert to float

# Test with masked exponent
a = Scalar([2., 3., 4.])
b = Scalar(2., mask=True)
c = a ** b
self.assertTrue(np.all(c.mask))

# Test with invalid result
a = Scalar([2., 3., 4.])
b = Scalar([1000., 1000., 1000.]) # Very large exponent
try:
c = a ** b
# May mask invalid values
except (ValueError, OverflowError):
pass

# Test with derivatives
a = Scalar([2., 3., 4.])
a.insert_deriv('t', Scalar([0.1, 0.2, 0.3]))
b = a ** 2
self.assertTrue(hasattr(b, 'd_dt'))

##################################################################################
# Additional tests for missing lines
##################################################################################

# Test as_scalar with Boolean.as_int() path
b = Boolean([True, False, True])
s = Scalar.as_scalar(b, recursive=False)
self.assertEqual(type(s), Scalar)

# Test as_index_and_mask with scalar values
a = Scalar(5)
idx, mask = a.as_index_and_mask()
self.assertEqual(idx, 5)
self.assertFalse(mask)

# Test as_index_and_mask with masked=None
a = Scalar([1, 2, 3])
idx, mask = a.as_index_and_mask(masked=None)
self.assertTrue(np.array_equal(idx, [1, 2, 3]))
self.assertFalse(mask)

# Test int() with top as list/tuple
a = Scalar([1.5, 2.5, 3.5])
b = a.int(top=[5])
self.assertTrue(np.all(b.values <= 4))

# Test int() with non-int values and mask copying
a = Scalar([1.5, 2.5, 3.5], mask=[False, True, False])
b = a.int(top=3)
self.assertTrue(isinstance(b._mask, np.ndarray))

# Test int() with shift and array values
a = Scalar([1., 2., 3.])
b = a.int(top=2, shift=True, clip=False)
# When shift=True and value==top, it becomes top-1
# Value 2 becomes 1, but value 3 stays 3 (no clip)
self.assertEqual(b.values[0], 1) # 1 stays 1
self.assertEqual(b.values[1], 1) # 2 becomes 1 (shifted)
self.assertEqual(b.values[2], 3) # 3 stays 3 (no clip)

# Test int() with clip and remask
a = Scalar([-1., 0., 1., 2.])
b = a.int(top=2, clip=True, remask=True)
self.assertTrue(np.all(b.values >= 0))
self.assertTrue(np.all(b.values < 2))

# Test int() with builtins
a = Scalar(1.5)
with prefer_builtins(True):
b = a.int(builtins=True)
self.assertIsInstance(b, int)

# Test frac() with denominators
# Scalar with drank=1 needs values with shape (..., 1)
a = Scalar([[1.5]], drank=1) # shape (1,), item (1,)
try:
_ = a.frac()
self.fail("Expected ValueError for frac() with denominators")
except ValueError:
pass

# Test sin() with denominators
a = Scalar([[1.0]], drank=1)
try:
_ = a.sin()
self.fail("Expected ValueError for sin() with denominators")
except ValueError:
pass

# Test cos() with denominators
a = Scalar([[1.0]], drank=1)
try:
_ = a.cos()
self.fail("Expected ValueError for cos() with denominators")
except ValueError:
pass

# Test tan() with denominators
a = Scalar([[1.0]], drank=1)
try:
_ = a.tan()
self.fail("Expected ValueError for tan() with denominators")
except ValueError:
pass

# Test arcsin() with denominators
a = Scalar([[0.5]], drank=1)
try:
_ = a.arcsin()
self.fail("Expected ValueError for arcsin() with denominators")
except ValueError:
pass

# Test arcsin() with RuntimeWarning
a = Scalar(1.5) # Outside domain
try:
with warnings.catch_warnings():
warnings.simplefilter("error", RuntimeWarning)
_ = a.arcsin(check=False)
except (ValueError, RuntimeWarning):
pass # Expected

# Test arccos() with denominators
a = Scalar([[0.5]], drank=1)
try:
_ = a.arccos()
self.fail("Expected ValueError for arccos() with denominators")
except ValueError:
pass

# Test arccos() with RuntimeWarning
a = Scalar(1.5) # Outside domain
try:
with warnings.catch_warnings():
warnings.simplefilter("error", RuntimeWarning)
_ = a.arccos(check=False)
except (ValueError, RuntimeWarning):
pass # Expected

# Test arctan() with denominators
a = Scalar([[1.0]], drank=1)
try:
_ = a.arctan()
self.fail("Expected ValueError for arctan() with denominators")
except ValueError:
pass

# Test arctan2() with denominators
a = Scalar([[1.0]], drank=1)
b = Scalar(1.0)
try:
_ = a.arctan2(b)
self.fail("Expected ValueError for arctan2() with denominators")
except ValueError:
pass

# Test sqrt() with denominators
a = Scalar([[4.0]], drank=1)
try:
_ = a.sqrt()
self.fail("Expected ValueError for sqrt() with denominators")
except ValueError:
pass

# Test log() with denominators
a = Scalar([[2.0]], drank=1)
try:
_ = a.log()
self.fail("Expected ValueError for log() with denominators")
except ValueError:
pass

# Test exp() with denominators
a = Scalar([[1.0]], drank=1)
try:
_ = a.exp()
self.fail("Expected ValueError for exp() with denominators")
except ValueError:
pass

# Test exp() with RuntimeWarning/ValueError
a = Scalar(1000.) # Very large value
try:
with warnings.catch_warnings():
warnings.simplefilter("error", RuntimeWarning)
_ = a.exp(check=False)
except (ValueError, RuntimeWarning):
pass # Expected

# Test sign() with builtins
a = Scalar(1.0)
with prefer_builtins(True):
b = a.sign(builtins=True)
self.assertIsInstance(b, float)

# Test solve_quadratic with include_antimask
a = Scalar([1., 2., 3.])
b = Scalar([-1., -2., -3.])
c = Scalar([0., 0., 0.])
_, _, discr = Scalar.solve_quadratic(a, b, c, include_antimask=True)
self.assertIsNotNone(discr)

# Test max() with empty size
a = Scalar([])
b = a.max()
# Empty array max() returns shape (0,)
self.assertEqual(b.shape, (0,))

# Test max() with mask handling
a = Scalar([1., 2., 3.], mask=[False, True, False])
b = a.max()
self.assertEqual(b, 3.)

# Test min() with empty size
a = Scalar([])
b = a.min()
self.assertEqual(b.shape, (0,))

# Test min() with mask handling
a = Scalar([1., 2., 3.], mask=[True, False, False])
b = a.min()
self.assertEqual(b, 2.)

# Test min() with builtins
a = Scalar([1., 2., 3.])
with prefer_builtins(True):
b = a.min(builtins=True)
self.assertIsInstance(b, float)

# Test argmax() with denominators
# Scalar with drank=1 needs values with shape (n, 1) for array of size n
a = Scalar([[1.], [2.], [3.]], drank=1) # shape (3,), item (1,)
try:
_ = a.argmax()
self.fail("Expected ValueError for argmax() with denominators")
except ValueError:
pass

# Test argmax() with empty size
# This may raise IndexError due to _zero_sized_result trying to index empty array
a = Scalar([])
try:
b = a.argmax()
self.assertEqual(b.shape, (0,))
except IndexError:
pass # Expected for empty array

# Test argmax() with mask handling
a = Scalar([1., 2., 3.], mask=[True, False, False])
b = a.argmax()
self.assertEqual(b, 2)

# Test argmax() with builtins
a = Scalar([1., 2., 3.])
with prefer_builtins(True):
b = a.argmax(builtins=True)
self.assertIsInstance(b, int)

# Test argmin() with denominators
a = Scalar([[1.], [2.], [3.]], drank=1)
try:
_ = a.argmin()
self.fail("Expected ValueError for argmin() with denominators")
except ValueError:
pass

# Test argmin() with empty size
# This may raise IndexError due to _zero_sized_result trying to index empty array
a = Scalar([])
try:
b = a.argmin()
self.assertEqual(b.shape, (0,))
except IndexError:
pass # Expected for empty array

# Test argmin() with mask handling
a = Scalar([1., 2., 3.], mask=[True, False, False])
b = a.argmin()
self.assertEqual(b, 1)

# Test argmin() with builtins
a = Scalar([1., 2., 3.])
with prefer_builtins(True):
b = a.argmin(builtins=True)
self.assertIsInstance(b, int)

##################################################################################
# Test argmax edge cases for coverage
##################################################################################
# Test argmax with partially masked array and scalar mask result
a = Scalar([[1., 2., 3.], [4., 5., 6.]])
mask = np.array([[False, False, False], [True, True, True]])
a_masked = Scalar(a.values, mask=mask)
result = a_masked.argmax(axis=1)
# Row 0 should have argmax, row 1 should be masked
self.assertIsInstance(result, Scalar)
self.assertEqual(result.shape, (2,))
self.assertFalse(result.mask[0])
self.assertTrue(result.mask[1])

# Test argmax with partially masked array and array mask result
a = Scalar([[1., 2., 3.], [4., 5., 6.], [7., 8., 9.]])
mask = np.array([[False, False, False], [True, True, True], [False, True, False]])
a_masked = Scalar(a.values, mask=mask)
result = a_masked.argmax(axis=1)
# Should handle array mask case
self.assertIsInstance(result, Scalar)
self.assertEqual(result.shape, (3,))

# Test argmax with scalar mask result
# Need case where np.all(self._mask, axis=axis) returns scalar True
# This happens when reducing to scalar shape
a = Scalar([1., 2., 3.], mask=[True, True, True])
result = a.argmax(axis=None)
# When all masked and axis=None, mask becomes scalar
self.assertIsInstance(result, Scalar)
# Verify the code path was executed
self.assertTrue(result.mask if isinstance(result.mask, (bool, np.bool_)) else np.all(result.mask))

##################################################################################
# Test argmin edge cases for coverage
##################################################################################
# Test argmin with partially masked array and scalar mask result
a = Scalar([[1., 2., 3.], [4., 5., 6.]])
mask = np.array([[False, False, False], [True, True, True]])
a_masked = Scalar(a.values, mask=mask)
result = a_masked.argmin(axis=1)
# Row 0 should have argmin, row 1 should be masked
self.assertIsInstance(result, Scalar)
self.assertEqual(result.shape, (2,))
self.assertFalse(result.mask[0])
self.assertTrue(result.mask[1])

# Test argmin with partially masked array and array mask result
a = Scalar([[1., 2., 3.], [4., 5., 6.], [7., 8., 9.]])
mask = np.array([[False, False, False], [True, True, True], [False, True, False]])
a_masked = Scalar(a.values, mask=mask)
result = a_masked.argmin(axis=1)
# Should handle array mask case
self.assertIsInstance(result, Scalar)
self.assertEqual(result.shape, (3,))

# Test argmin with scalar mask result
# Need case where np.all(self._mask, axis=axis) returns scalar True
# This happens when reducing to scalar shape
a = Scalar([1., 2., 3.], mask=[True, True, True])
result = a.argmin(axis=None)
# When all masked and axis=None, mask becomes scalar
self.assertIsInstance(result, Scalar)
# Verify the code path was executed
self.assertTrue(result.mask if isinstance(result.mask, (bool, np.bool_)) else np.all(result.mask))

# Test maximum() with denominators
a = Scalar([[1.], [2.], [3.]], drank=1)
b = Scalar([2., 3., 4.])
try:
_ = Scalar.maximum(a, b)
self.fail("Expected ValueError for maximum() with denominators")
except ValueError:
pass

# Test minimum() with denominators
a = Scalar([[1.], [2.], [3.]], drank=1)
b = Scalar([2., 3., 4.])
try:
_ = Scalar.minimum(a, b)
self.fail("Expected ValueError for minimum() with denominators")
except ValueError:
pass

# Test median() with denominators
a = Scalar([[1.], [2.], [3.]], drank=1)
try:
_ = a.median()
self.fail("Expected ValueError for median() with denominators")
except ValueError:
pass

# Test median() with empty size
# This may raise IndexError due to _zero_sized_result trying to index empty array
a = Scalar([])
try:
b = a.median()
self.assertEqual(b.shape, (0,))
except IndexError:
pass # Expected for empty array

# Test median() with mask handling
a = Scalar([1., 2., 3., 4., 5.], mask=[True, False, False, False, True])
b = a.median()
self.assertIsNotNone(b)

# Test median() with builtins
a = Scalar([1., 2., 3.])
with prefer_builtins(True):
b = a.median(builtins=True)
self.assertIsInstance(b, float)

# Test sort() with denominators
a = Scalar([[3.], [1.], [2.]], drank=1)
try:
_ = a.sort()
self.fail("Expected ValueError for sort() with denominators")
except ValueError:
pass

# Test sort() with empty size
# This may raise IndexError due to _zero_sized_result trying to index empty array
a = Scalar([])
try:
b = a.sort()
self.assertEqual(b.shape, (0,))
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider splitting this massive runTest into smaller, focused test methods.

Test_Scalar_Coverage.runTest is exercising a huge number of orthogonal behaviors: indexing/as_scalar/int/frac/trig, all the _check paths, denominator errors, broadcasting, median/arg edge cases, power helpers, masking/derivative propagation, prefer_builtins, etc. When a single assertion fails, it’s hard to see which feature regressed.

Refactoring into grouped methods (e.g., test_minmax_and_as_scalar, test_int_and_frac_variants, test_trig_with_check_flags, test_aggregation_and_arg_extrema, test_power_helpers_and_derivs, test_builtins_preference) would make failures much easier to diagnose without changing coverage intent.

🧰 Tools
🪛 Ruff (0.14.8)

52-52: Use a regular assert instead of unittest-style assertIsNotNone

Replace assertIsNotNone(...) with assert ...

(PT009)


53-53: Use a regular assert instead of unittest-style assertIsNotNone

Replace assertIsNotNone(...) with assert ...

(PT009)


59-59: Use a regular assert instead of unittest-style assertIsNotNone

Replace assertIsNotNone(...) with assert ...

(PT009)


60-60: Use a regular assert instead of unittest-style assertIsNotNone

Replace assertIsNotNone(...) with assert ...

(PT009)


66-66: Boolean positional value in function call

(FBT003)


68-68: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


77-77: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


83-83: Use a regular assert instead of unittest-style assertIsNotNone

Replace assertIsNotNone(...) with assert ...

(PT009)


89-89: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


96-96: Use pytest.raises instead of unittest-style assertRaises

(PT027)


102-102: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


109-109: Use pytest.raises instead of unittest-style assertRaises

(PT027)


121-121: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


127-127: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


132-132: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


138-138: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


157-157: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


162-162: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


167-167: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


172-172: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


176-176: Boolean positional value in function call

(FBT003)


178-178: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


189-189: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


195-195: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


206-206: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


216-216: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


226-226: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


236-236: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


250-250: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


260-260: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


274-274: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


284-284: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


295-295: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


305-305: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


324-324: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


343-343: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


365-365: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


369-369: Boolean positional value in function call

(FBT003)


376-376: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


377-377: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


378-378: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


388-388: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


393-393: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


399-399: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


403-403: Boolean positional value in function call

(FBT003)


405-405: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


415-415: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


420-420: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


426-426: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


430-430: Boolean positional value in function call

(FBT003)


432-432: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


442-442: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


446-446: Use pytest.raises instead of unittest-style assertRaises

(PT027)


451-451: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


461-461: Boolean positional value in function call

(FBT003)


463-463: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


473-473: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


477-477: Use pytest.raises instead of unittest-style assertRaises

(PT027)


482-482: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


492-492: Boolean positional value in function call

(FBT003)


494-494: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


500-500: Use pytest.raises instead of unittest-style assertRaises

(PT027)


508-508: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


513-513: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


519-519: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


525-525: Use pytest.raises instead of unittest-style assertRaises

(PT027)


533-533: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


538-538: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


544-544: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


554-554: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


559-559: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


575-575: Boolean positional value in function call

(FBT003)


577-577: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


587-587: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


604-604: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


618-618: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


629-629: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


697-697: Boolean positional value in function call

(FBT003)


699-699: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


701-701: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


703-703: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


705-705: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


712-712: Use a regular assert instead of unittest-style assertAlmostEqual

(PT009)


720-720: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


729-729: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


738-738: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


740-740: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


748-748: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


749-749: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


752-752: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


753-753: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


756-756: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


757-757: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


765-765: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


766-766: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


769-769: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


770-770: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


773-773: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


774-774: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


782-782: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


786-786: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


790-790: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


794-794: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


798-798: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


802-802: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


806-806: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


810-810: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


816-816: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


822-822: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


837-837: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


846-846: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


851-851: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


852-852: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


857-857: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


858-858: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


863-863: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


868-868: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


875-875: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


876-876: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


877-877: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


882-882: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


883-883: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


887-887: Boolean positional value in function call

(FBT003)


889-889: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1010-1010: Boolean positional value in function call

(FBT003)


1012-1012: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1019-1019: Use a regular assert instead of unittest-style assertIsNotNone

Replace assertIsNotNone(...) with assert ...

(PT009)


1025-1025: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1030-1030: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1035-1035: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1040-1040: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1044-1044: Boolean positional value in function call

(FBT003)


1046-1046: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1062-1062: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1069-1069: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1073-1073: Boolean positional value in function call

(FBT003)


1075-1075: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1090-1090: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1097-1097: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1101-1101: Boolean positional value in function call

(FBT003)


1103-1103: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1114-1114: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1115-1115: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1116-1116: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


1117-1117: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


1125-1125: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1126-1126: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1134-1134: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1136-1136: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


1147-1147: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1148-1148: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1149-1149: Use a regular assert instead of unittest-style assertFalse

Replace assertFalse(...) with assert ...

(PT009)


1150-1150: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


1158-1158: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1159-1159: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1167-1167: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1169-1169: Use a regular assert instead of unittest-style assertTrue

Replace assertTrue(...) with assert ...

(PT009)


1202-1202: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


1209-1209: Use a regular assert instead of unittest-style assertIsNotNone

Replace assertIsNotNone(...) with assert ...

(PT009)


1213-1213: Boolean positional value in function call

(FBT003)


1215-1215: Use a regular assert instead of unittest-style assertIsInstance

Replace assertIsInstance(...) with assert ...

(PT009)


1230-1230: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)

Comment on lines +35 to +46
try:
dtype = np.dtype('U') # Unicode string dtype
_ = Scalar._minval(dtype)
except ValueError:
pass # Expected

try:
dtype = np.dtype('U')
_ = Scalar._maxval(dtype)
except ValueError:
pass # Expected

Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Replace naked try/except ... pass with explicit exception assertions.

In several places you expect an error but the test will also pass if nothing is raised, e.g.:

  • _minval/_maxval with dtype='U' (lines 35–46),
  • Scalar.as_scalar(Vector(...)) rank mismatch (lines 73–80),
  • Vector(..., drank=1).int() (lines 143–148),

and similar patterns later in the file (various sqrt/log/exp/arcsin/arccos “error cases”).

For these, prefer:

with self.assertRaises(ValueError):
    Scalar._minval(np.dtype('U'))

or use self.fail("Expected ...") after the call if you need to allow multiple exception types. That way the tests fail if the implementation silently changes behavior instead of just exercising lines.

Also applies to: 73-80, 143-148

🤖 Prompt for AI Agents
tests/test_scalar_coverage.py lines 35-46, 73-80, 143-148 (and other similar
"expect error" blocks): replace the naked try/except that just passes with
explicit test assertions that an exception is raised; for single expected
exception types wrap the call in with self.assertRaises(ValueError): and call
the function inside that context, and if multiple exception types are acceptable
use try/except but call self.fail("Expected ...") immediately after the call so
the test fails if no exception is raised; apply this pattern to the listed
ranges and any other similar sqrt/log/exp/arcsin/arccos error-case blocks in the
file.

@rfrenchseti
Copy link
Collaborator Author

@CodeRabbit summary

@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

✅ Actions performed

Summary regeneration triggered.

@rfrenchseti
Copy link
Collaborator Author

@CodeRabbit summary

@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

✅ Actions performed

Summary regeneration triggered.

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.

2 participants