Repository navigation
test: characterize SScalar and document what it is - #84
Merged
Merged
Conversation
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
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
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
SScalaris 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 theDDScalarside earlier in this series.This is step 1 of three: characterize, then change the storage (#next), then second order.
What
test/src/test_sscalar.cppcovers 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,evalagainst a displacement, whatsizecounts, 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 thatcopy,deepcopyandpickleall raise.Validated by mutation
Five deliberate faults in the header, each failing three assertions:
cbrtderivative 3 → 2acosloses its signatanhdenominator signlog10on the wrong basesinhderivativecosh→sinhTwo fixes, both prerequisites rather than scope
m_fwas indeterminate. The defaulted constructor left it uninitialized, so a test assertingS().f() == 0would have been testing luck rather than behaviour. Initialized now.evalcopied the name on every iteration. clang-tidy had never reported it because nothing instantiatedeval— 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:Two findings recorded rather than changed
operator<<is unspecified, because the derivatives live in anunordered_map. The tests assert the value comes first and that every term appears — not the whole string.s1 - s1hassize() == 3, not 0.Documentation
The README said nothing about any of this. It now states that
SScalaris 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 whatevaldoes with names present on only one side. Every example number was checked against the module.Verification
-Werror, and under clang with ASan+UBSanclang-format --Werrorandruff format --checkclean.clang-tidydropsmodernize-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 flaggedcbrt(3)aslog2ewhile itself reporting that the two differ by 4.45e-04.