Skip to content

test: characterize SScalar and document what it is - #84

Merged
oberbichler merged 1 commit into
mainfrom
test/characterize-sscalar
Jul 27, 2026
Merged

oberbichler merged 1 commit into
mainfrom
test/characterize-sscalar

Conversation

@oberbichler

Copy link
Copy Markdown
Owner

Why

SScalar is 746 lines with 20 C++ test cases covering the operators and a handful of functions, and 4 Python tests. The plan is to change its storage — and without a net, a change of that size cannot be verified, only hoped for. It is the same gap that let three memory-safety bugs through on the DDScalar side earlier in this series.

This is step 1 of three: characterize, then change the storage (#next), then second order.

What

test/src/test_sscalar.cpp covers the functions the existing cases miss — reciprocal, cbrt, the inverse trigonometric and all hyperbolic functions, the exponential and logarithmic family, abs — plus the behaviour nothing pinned at all: construction and the factories, comparison by value only, eval against a displacement, what size counts, and printing. Expected values derived symbolically, so they share no formulas with the header.

Python goes from 4 tests to 12, aimed at what the bindings add rather than at the mathematics: the operator set including the reverse and in-place forms, the NumPy aliases against their primary names, __len__, __abs__, __pow__, __repr__, and that copy, deepcopy and pickle all raise.

Validated by mutation

Five deliberate faults in the header, each failing three assertions:

mutation
cbrt derivative 3 → 2 3 failed
acos loses its sign 3 failed
atanh denominator sign 3 failed
log10 on the wrong base 3 failed
sinh derivative cosh → sinh 3 failed

Two fixes, both prerequisites rather than scope

m_f was indeterminate. The defaulted constructor left it uninitialized, so a test asserting S().f() == 0 would have been testing luck rather than behaviour. Initialized now.

eval copied the name on every iteration. clang-tidy had never reported it because nothing instantiated eval — the new tests do, and the warning appeared immediately. That is the same instantiation-coverage effect as in #75 and #79, this time with a measurable defect as the payoff. With 20 names past the small-string limit:

before:  1657 ns, 20.00 allocations per call
after:    287 ns,  0.00 allocations per call

Two findings recorded rather than changed

  • The order of terms in operator<< is unspecified, because the derivatives live in an unordered_map. The tests assert the value comes first and that every term appears — not the whole string.
  • Names survive an operation even where the derivative cancels: s1 - s1 has size() == 3, not 0.

Documentation

The README said nothing about any of this. It now states that SScalar is first order only, that the variable set does not have to be known in advance, that an unknown name is zero rather than an error, that there is no serialization, and what eval does with names present on only one side. Every example number was checked against the module.

Verification

  • C++ 112 test cases, 1425 assertions — Debug and Release with -Werror, and under clang with ASan+UBSan
  • Python 248 passed
  • clang-tidy back to 50 findings, clang-format --Werror and ruff format --check clean

.clang-tidy drops modernize-use-std-numbers: it cannot tell an expected test value that happens to sit near a constant from a constant written out by hand, and it flagged cbrt(3) as log2e while itself reporting that the two differ by 4.45e-04.

SScalar is 746 lines with 20 C++ test cases covering the operators and a
handful of functions, and 4 Python tests. That is not a base to redesign from:
without a net a change to the storage cannot be verified. It is the same gap
that let three memory-safety bugs through on the DDScalar side.

Adds test/src/test_sscalar.cpp with the functions the existing cases miss --
reciprocal, cbrt, the inverse trigonometric and all hyperbolic functions, the
exponential and logarithmic family, abs -- plus the behaviour nothing pinned:
construction and the factories, comparison by value only, eval against a
displacement, what size counts, and printing. Expected values derived
symbolically, so they share no formulas with the header.

Python side goes from 4 tests to 12, covering what the bindings add rather than
the mathematics: the operator set including the reverse and in-place forms, the
NumPy aliases against their primary names, __len__, __abs__, __pow__, __repr__,
and that copy, deepcopy and pickle all raise.

Validated by mutation. Five deliberate faults in the header -- cbrt's
derivative 3 -> 2, acos losing its sign, atanh's denominator sign, log10 on the
wrong base, sinh's derivative cosh -> sinh -- each fail three assertions.

Two things fixed along the way, both prerequisites rather than scope:

m_f was left indeterminate by the defaulted constructor, so a test for the
default-constructed value would have been testing luck. It is initialized now.

eval copied the pair, and with it the name, on every iteration. clang-tidy had
never seen it because nothing instantiated eval -- the new tests do, and the
warning appeared. With 20 names past the small-string limit, eval went from
1657 ns and 20 allocations per call to 287 ns and none.

Two characterization findings worth recording rather than changing:

The order of the terms in operator<< is unspecified, because the derivatives
live in an unordered_map. The tests assert the value comes first and that every
term appears, not the whole string.

Names survive an operation even where the derivative cancels: s1 - s1 has size
3, not 0.

README gains what was missing entirely: that SScalar is first order only, that
the variable set does not have to be known in advance, that an unknown name is
zero rather than an error, that there is no serialization, and what eval does
with names on only one side. All example numbers checked against the module.

.clang-tidy drops modernize-use-std-numbers. It cannot tell an expected test
value that happens to sit near a constant from a constant written out by hand,
and it flagged cbrt(3) as log2e while itself reporting that the two differ.

  C++ 112 test cases, 1425 assertions, Debug and Release with -Werror and
  under clang with ASan+UBSan
  Python 248 passed
  clang-tidy back to 50 findings, clang-format and ruff format clean
@oberbichler
oberbichler merged commit d074ce5 into main Jul 27, 2026
17 checks passed
@oberbichler
oberbichler deleted the test/characterize-sscalar branch July 27, 2026 15:48
oberbichler added a commit that referenced this pull request Jul 27, 2026
A multiply of two single-entry values cost six allocations, eval copied the
name on every iteration, and binary and ternary copied the whole map. The
storage becomes a vector of name and derivative sorted by name, so combining
two values is a linear merge rather than a sequence of hash lookups and node
allocations. Measured on an expression chain, -O3:

     names        before              after
         2   416 ns,  12 allocs   86 ns,  2 allocs    4.8x
         4  2029 ns,  53 allocs  280 ns,  6 allocs    7.2x
        12 14271 ns, 375 allocs 1546 ns, 22 allocs    9.2x

This replaces the design proposed for this step. Interning the names behind a
shared table was going to be the approach, and the allocation accounting did
not survive scrutiny: with different name sets the merge and the remap of both
operands cost no less than the map, and in (x*y)/(x-y) the tables are equal in
content but not in identity, so the fast path would never have fired. Both
designs were prototyped and measured before choosing.

The sorted vector is not a detour for second order either. The position of a
name in the vector is its index, which is what a dense triangular Hessian would
be laid out over, and the merge already produces the union in sorted order.

Data stays the map. It is the interface type for construction and eval, which
is what callers and in particular a Python dict naturally provide; the vector
is internal. A second constructor taking the internal type would have made
SScalar(f, {{"x", 1.0}}) ambiguous between the two, so the operations go through
a named factory instead.

Three simplifications fall out. The four in-place operators delegate to their
binary form, which puts the merge in one place and makes aliased operands
correct without the guards added in #76. The derivatives are now iterated in a
defined order, so operator<< and repr are deterministic instead of unspecified
-- the tests and the README are tightened accordingly. And clang-tidy drops
from 50 findings to 41, because the braceless single statements it complained
about were in the code that went away.

Also replaces the int coefficients in operator+ and operator- with Scalar,
which matters for a float scalar type.

The public API is unchanged: 35 bound methods as before, SScalar(f=, d={...}),
size, d and eval identical. That the characterization from #84 carries the
rewrite without a single expected value being touched is the evidence that
doing it first was right.

  C++ 112 test cases, 1421 assertions, Debug and Release with -Werror and
  under clang with ASan+UBSan
  Python 248 passed
  clang-format and ruff format clean
oberbichler added a commit that referenced this pull request Jul 27, 2026
…ap (#85)

A multiply of two single-entry values cost six allocations, eval copied the
name on every iteration, and binary and ternary copied the whole map. The
storage becomes a vector of name and derivative sorted by name, so combining
two values is a linear merge rather than a sequence of hash lookups and node
allocations. Measured on an expression chain, -O3:

     names        before              after
         2   416 ns,  12 allocs   86 ns,  2 allocs    4.8x
         4  2029 ns,  53 allocs  280 ns,  6 allocs    7.2x
        12 14271 ns, 375 allocs 1546 ns, 22 allocs    9.2x

This replaces the design proposed for this step. Interning the names behind a
shared table was going to be the approach, and the allocation accounting did
not survive scrutiny: with different name sets the merge and the remap of both
operands cost no less than the map, and in (x*y)/(x-y) the tables are equal in
content but not in identity, so the fast path would never have fired. Both
designs were prototyped and measured before choosing.

The sorted vector is not a detour for second order either. The position of a
name in the vector is its index, which is what a dense triangular Hessian would
be laid out over, and the merge already produces the union in sorted order.

Data stays the map. It is the interface type for construction and eval, which
is what callers and in particular a Python dict naturally provide; the vector
is internal. A second constructor taking the internal type would have made
SScalar(f, {{"x", 1.0}}) ambiguous between the two, so the operations go through
a named factory instead.

Three simplifications fall out. The four in-place operators delegate to their
binary form, which puts the merge in one place and makes aliased operands
correct without the guards added in #76. The derivatives are now iterated in a
defined order, so operator<< and repr are deterministic instead of unspecified
-- the tests and the README are tightened accordingly. And clang-tidy drops
from 50 findings to 41, because the braceless single statements it complained
about were in the code that went away.

Also replaces the int coefficients in operator+ and operator- with Scalar,
which matters for a float scalar type.

The public API is unchanged: 35 bound methods as before, SScalar(f=, d={...}),
size, d and eval identical. That the characterization from #84 carries the
rewrite without a single expected value being touched is the evidence that
doing it first was right.

  C++ 112 test cases, 1421 assertions, Debug and Release with -Werror and
  under clang with ASan+UBSan
  Python 248 passed
  clang-format and ruff format clean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant