fix: a few more findings from a codebase review by ariostas · Pull Request #96 · scikit-hep/formulate

ariostas · 2026-07-31T14:02:29Z

These were a few more things that were found after a review with GPT 5.6 Sol.

The most important change is the dotted names one. I had some discussions with Jonas about this a while ago and we temporarily decided to keep dots in numexpr outputs even though they were not valid. But I've been thinking about it more, and I think the right approach is to actually output valid numexpr expressions. For this, I borrowed the idea from Uproot of converting invalid symbols (in this case only .) into their hex representation surrounded by parenthesis. This makes a lot of sense to me, and is what we will use in Uproot once we integrate Formulate as the expression parser.

🤖 AI text below 🤖

Acts on an external review of the codebase. Each finding was reproduced before
being fixed, and a few were rejected — see the end.

Every commit is independent and the branch is green throughout: 1,993 tests
pass, coverage is 100% for both statements and branches, prek run --all-files
is clean, and nox (lint + pylint + tests) and nox -s docs both succeed.

Correctness

Constants were not actually being checked. assert_same_value and
test_constant_translations compared with np.isclose's default atol of
1e-8, which is larger than several of the constants they check. Every constant
below 1e-8 therefore compared equal to every other one, and to zero. Two
expectations had already drifted behind that gap: hbar held h_planck's
value, copy-pasted from the line above, and hbarc was off by ten orders of
magnitude. The library's own values were correct — only the tests were wrong,
and test_constants.py's ROOT-vs-NumExpr cross-check was passing vacuously for
six constants. Now compared relatively with no absolute floor. The bound is
1e-8 rather than something tighter because ROOT hardcodes CODATA's rounded
1.054571817e-34 while hepunits divides the exact h by 2π — a real 6e-10
disagreement, and the only one in the file; everything else is bit-exact.

AST nodes are no longer comparable or hashable (breaking, though the
methods were only ever the dataclass-generated defaults). The review flagged
that == and hash() recursed — both compare the field tuple, and a field
holding a child node makes that a recursive descent, so a tree that serialized
fine raised RecursionError at ~1,200 levels. Making them iterative fixed the
symptom but left the real problem: structural equality is not the question
anyone is asking. It calls a + b and b + a different expressions, along
with x * 1 and x, (a + b) + c and a + (b + c), and any constant spelled
differently from the one parsed. Answering those properly is computer algebra,
well outside what a syntax translator can promise, so the package no longer
implies an answer: __eq__ raises TypeError naming the alternative, and
__hash__ is None so nodes are honestly not Hashable.

Compare serializations instead — str(node) for the canonical form, or a
to_* rendering. Those are deterministic and fully parenthesized, so equal
strings mean identically shaped trees, which is what most of the suite already
relied on.

Dotted branch names produced unusable NumExpr. branch.leaf is one ROOT
name, not an attribute access, and NumExpr rejects any expression containing a
dot. These were emitted unchanged. They are now hex-encoded the way uproot
encodes C++ class names — branch.leaf → branch_2e_leaf — using uproot's
pattern verbatim, so the two packages spell a given name identically and
uproot's own decoder inverts it. Encoding is deliberately one-way: from_numexpr
cannot distinguish an encoded branch.leaf from a branch genuinely named
branch_2e_leaf, and silently renaming the latter would be worse than not
restoring the former. Note that variables still reports ROOT's spelling, so
the docs now say it is the encoded name that must be supplied at evaluation.

: was accepted below the top level. It is never parenthesized — that is
the point, since it separates outputs rather than combining values — so a
nested one could not be written back out: (a:b)+c serialized as a : b + c
and re-parsed as a:(b+c), silently changing which expressions a TTree::Draw
would produce. ROOT does not accept a nested one either, so it is now
restricted to the top of the grammar, with a ParseError suggestion that keys
off a lone colon so TMath::Sqrt does not trigger it.

complex(a, b) rendered as np.complex128(a, b), which is a scalar type
constructor rather than the element-wise counterpart: it raises TypeError on
array input, which is what these expressions are almost always evaluated
against. NumPy's element-wise form is a + 1j*b, an expression rather than a
name, so the mapping is dropped and the missing entry raises — exactly as
contains already does for the same reason.

Overflowing literals emitted a bare inf, which no target accepts.
1e999 is now read as the canonical inf constant, which already knows
TMath::Infinity() for ROOT, float('inf') for Python, and that NumExpr has
no spelling at all. Finite literals are untouched.

Release safety

deploy depended only on dist, so publishing required nothing more than that
a wheel could be built — a release whose tests had failed would still reach
PyPI. unittests is now callable and CD invokes it, making the whole matrix a
prerequisite. It is skipped outside a release, since that workflow already runs
on every push and PR. Only CODECOV_TOKEN is passed rather than inheriting
every secret.

Added a smoke test that installs the built wheel and sdist and runs the suite
against them. Tests from a checkout exercise the source tree and cannot catch a
packaging mistake — the grammars are package data, and a wheel missing them
would pass every existing test and fail on import for users. This one runs on
PRs too.

Housekeeping

__slots__ = () on the AST base. The node types were already slots=True,
but a slotted class inheriting from an unslotted one still gets a __dict__,
so the declaration was buying nothing.
Coverage config moved from .coveragerc into pyproject.toml, with branch
coverage and fail_under = 100 — matching what codecov already required, so
a gap fails locally instead of only after a push. Coverage reads .coveragerc
in preference to pyproject.toml, so with both present the latter is
silently ignored; one file now. Verified the suite reaches 100% without ROOT,
so every job in the matrix can meet it.
nox -s build now clears dist/, where python -m build actually writes,
instead of build/, which hatchling never creates.
nox -s pylint passes (10.00/10); it had been failing on main, so nox
itself was red. The one real fix is naming data/children in the
lark.Tree patterns rather than matching positionally, since __match_args__
is a hand-written tuple on a non-dataclass. The rest are narrow suppressions
with reasons.
Fixed a latent assume in the invalid-character fuzz test, which allowlisted
only two whitespace characters though the grammar ignores all of them.

Deliberately not addressed

ROOT && vs NumExpr & on non-boolean operands. Real, but the binding
difference is already documented, and NumExpr raises NotImplementedError on
integer arrays rather than answering wrongly. The suggested fix — distinct
canonical nodes — would fight the AST's central invariant that all three
spellings collapse to one node.
Serialization cost on skewed trees. Measured ~n^1.5, not quadratic;
32,000 terms serialize in 172 ms.
Parsers accept the other dialect's functions. Real laxness, but errors are
deferred rather than lost — from_root("where(a,b,c)").to_root() does raise.
Widening the Python matrix. Judged not worth the CI time.

`assert_same_value` and `test_constant_translations` both used `np.isclose` with its default `atol` of 1e-8, which is larger than several of the constants they check. Every constant below 1e-8 therefore compared equal to every other one, and to zero, so the checks that are supposed to pin their values down asserted nothing. Two expectations had already drifted behind that gap: `hbar` was h_planck's value copy-pasted from the line above, and `hbarc` was hepunits' natural-units figure with the wrong exponent, off by ten orders of magnitude. The library's own values were correct; only the tests were wrong. Compare relatively with no absolute floor. The bound is 1e-8 rather than something tighter because ROOT hardcodes CODATA's rounded 1.054571817e-34 for hbar while hepunits divides the exact h by 2*pi, a real 6e-10 disagreement; every other comparison in the file is bit-exact. Assisted-by: claude-code:claude-opus-5[1m]

Every walk in the package uses an explicit stack so that depth is bounded by memory rather than by the interpreter stack -- except the two nobody wrote. `@dataclass(frozen=True)` generates `__eq__` and `__hash__` that compare and hash the field tuple, and since a field can hold a child node, both descend one frame per level. `formulate.from_root("sqrt(" * 1200 + ...)` therefore serializes fine but raises RecursionError on `==` or `hash()`, contradicting both the stated depth guarantee and the hashability the class documents. Declare the nodes `eq=False` and implement both on the base class: `__eq__` walks two trees on a stack of pairs, `__hash__` folds bottom-up through the same `_traversal.fold` the serializers use. Each node type gains a `_key()` returning its own non-child data, which is what the two compare and mix in. Observable behaviour is unchanged -- including `Literal(1) == Literal(1.0)`, which the field-tuple comparison also considered equal. Assisted-by: claude-code:claude-opus-5[1m]

`literal_eval("1e999")` returns float infinity, which became a `Literal` whose serialization was a bare `inf`. None of the three target languages accept that: it is a NameError in Python, and neither ROOT nor NumExpr has such a spelling. Infinity is already a canonical constant, with `TMath::Infinity()` for ROOT, `float('inf')` for Python and no NumExpr spelling at all, so hand an overflowing literal to that machinery rather than emitting a number no engine can read back. It now shows up under `named_constants`, and `to_numexpr()` raises the same ValueError `TMath::Infinity()` already did. Finite literals are untouched: 1e300 is still a `Literal`. Assisted-by: claude-code:claude-opus-5[1m]

ROOT branch names routinely contain a dot -- `branch.leaf` is one name, not an attribute access -- but NumExpr rejects any expression containing one ("forbidden control characters") and has no quoting syntax to get around it, so these were emitted unchanged and produced NumExpr that could never be evaluated. Since ROOT -> NumExpr is the direction this library mostly gets used in, that was easy to hit. Encode the name the way uproot encodes C++ class names: each run of characters that cannot appear in an identifier becomes those bytes in hex, wrapped in underscores, so `branch.leaf` becomes `branch_2e_leaf`. The pattern is uproot's verbatim, so the two packages spell a given name identically and uproot's own decoder inverts this. A dot is the only character that can reach the serializer -- `toast` requires every dot-separated part of a symbol to be a Python identifier -- so that is what triggers encoding. Underscores are still escaped inside an encoded name, which keeps `a.b_c` (a_2e_b_5f_c) distinct from a branch really called `a_2e_b_c`. Names NumExpr can already spell are passed through untouched. The encoding is deliberately one-way. `from_numexpr` cannot distinguish an encoded `branch.leaf` from a branch genuinely named `branch_2e_leaf`, and silently renaming the latter would be worse than not restoring the former, so a round trip keeps the encoded spelling -- as it already does for the numeric value of a named constant. `variables` still reports ROOT's spelling, since the AST is canonical; the docs say so, because it is the encoded name that has to be supplied when the expression is evaluated. Assisted-by: claude-code:claude-opus-5[1m]

The grammar made ':' an ordinary binary operator, so it was accepted inside parentheses and function arguments. Serialization never parenthesizes it -- that is the point, since ':' separates outputs rather than combining values -- so a nested one could not be written back out: `(a:b)+c` serialized as `a : b + c`, which re-parses as `a:(b+c)`. A round trip silently changed which expressions a TTree::Draw would produce. ':' is how TTree::Draw separates whole expressions, and ROOT does not accept a nested one either, so restrict it to the top of the grammar. `a:b`, `a:b:c` and `a+1:b*2` are unaffected, including their associativity; `(a:b)+c` and `sqrt(a:b)` are now ParseErrors, with a suggestion explaining where ':' may appear. The suggestion keys off a lone colon so that TMath::Sqrt does not trigger it. Assisted-by: claude-code:claude-opus-5[1m]

`np.complex128` is a scalar type constructor, not the element-wise counterpart of NumExpr's `complex(a, b)`: it accepts 0-d input only and raises `TypeError` on arrays, which is what these expressions are almost always evaluated against. `to_python()` therefore produced a call that worked for single values and blew up on the array case. NumPy's element-wise form is `a + 1j*b`, an expression rather than a name, and `PYTHON_FUNCTIONS` maps canonical names to single names. So drop the mapping and let the missing entry raise, exactly as `contains` already does for the same reason. NumExpr keeps its own spelling; ROOT never had one. Assisted-by: claude-code:claude-opus-5[1m]

`test_invalid_characters` asserts that text made only of punctuation never parses, and excluded the known-parseable cases with a literal allowlist that covered '\r' and ' ' but no other whitespace. The grammar ignores all of it, so hypothesis eventually drew '\x0c_', which reduces to the bare symbol `_` and parses fine. Test the property instead of listing two of its instances. The TODO asking why `_` does not fail is answered in the comment rather than left open: it is a valid symbol on its own. Pre-existing; hypothesis just had not drawn a counterexample before. Assisted-by: claude-code:claude-opus-5[1m]

The deploy job depended only on `dist`, so publishing required nothing more than that a wheel could be built. Tests live in a separate workflow, and GitHub cannot express a dependency across workflows, so nothing connected the two: a release whose tests had failed would still go to PyPI. Make `unittests` callable and have CD invoke it, so the whole matrix is a prerequisite of `deploy`. It is skipped outside a release, since that workflow already runs on every push and pull request and duplicating it would only burn runners. Only CODECOV_TOKEN is passed rather than inheriting every secret. Add a smoke test that installs the built wheel and sdist and runs the suite against them. Tests from a checkout exercise the source tree, so they cannot catch a packaging mistake -- the grammars are package data, and a wheel missing them would pass every existing test and fail on import for users. This one runs on pull requests too, where that regression would be introduced. Assisted-by: claude-code:claude-opus-5[1m]

The six node types are all `slots=True` dataclasses, but a slotted class that inherits from an unslotted one still gets a `__dict__`, so every node in every tree carried one anyway and the declaration bought nothing. Assisted-by: claude-code:claude-opus-5[1m]

`python -m build` writes its output into dist/ and does not clear it first, so wheels and sdists from earlier versions accumulate there and get picked up by anything that globs dist/*. The session cleared build/ instead, which hatchling never creates -- a leftover from a setuptools-shaped template, so in practice it deleted nothing. Assisted-by: claude-code:claude-opus-5[1m]

codecov required 100% for both project and patch, but nothing did locally: a gap only surfaced after a push. Turn on branch coverage and set `fail_under = 100`, so `pytest --cov=formulate`, `nox -s coverage` and CI all hold a change to the standard codecov will apply anyway. Move the config from .coveragerc into pyproject.toml and delete the former. Coverage reads .coveragerc in preference to pyproject.toml, so with both present the settings in pyproject.toml are silently ignored -- which is exactly what happened while writing this. One file, and the same one that already holds the pytest, mypy, ruff and pylint config. The `raise NotImplementedError` exclusion is dropped rather than carried over. Both raises sit under an `else:` already marked `# pragma: no cover`, which is how this package spells unreachable code, and coverage stays at 100% with branches without it. Verified the suite reaches 100% with ROOT absent, so this is met by every job in the matrix and not only the one that uploads coverage. Assisted-by: claude-code:claude-opus-5[1m]

`nox` runs lint, pylint and tests by default, and the pylint session had been failing for a while — 9.65/10 on main — so the default entry point was red and any new finding was hidden in the noise. Four distinct complaints, fixed or silenced on their merits rather than in bulk. Real fix, in toast.py: the `case lark.Tree(...)` patterns matched `data` and `children` positionally, which depends on lark's `__match_args__` ordering. `lark.Tree` is not a dataclass — that tuple is hand-written and could be reordered without it being an API break — so name the two fields. It also reads better for anyone who does not know the class. The rest cannot be fixed and are suppressed narrowly: - `type(item) is tuple` in `_traversal.py` is exact on purpose; isinstance would mistake a tuple-subclassing node for a pending builder. Disabled on the line, with the reason spelled out in the comment that was already there. - `formulate.AST` is public API, so the module cannot be renamed to snake_case. `module-rgx` now allows that one name and nothing else — `BadName`, `Foo` and even `ASTfoo` are still flagged. - `_children`/`_key`/`_format`/`_serializer` are private to the package, not to the instance: the base class and `fold` call them on nodes other than self, which is what keeps every traversal in one place. Added to `exclude-protected`, alongside pylint's own defaults. Assisted-by: claude-code:claude-opus-5[1m]

codecov · 2026-07-31T14:04:07Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a7a7266) to head (ad5d046).
✅ All tests successful. No failed tests found.

Additional details and impacted files

Files with missing lines	Coverage Δ
src/formulate/AST.py	`100.00% <100.00%> (ø)`
src/formulate/_traversal.py	`100.00% <100.00%> (ø)`
src/formulate/exceptions.py	`100.00% <100.00%> (ø)`
src/formulate/identifiers.py	`100.00% <ø> (ø)`
src/formulate/toast.py	`100.00% <100.00%> (ø)`

CD calls the test matrix purely as a gate on publishing. It reports a commit codecov already has from the push or pull request that produced it, so the upload is redundant, and it was the only reason the call needed a secret. Skip the two codecov steps when the caller's event is a release, drop the `secrets:` block from the call, and drop the `workflow_call` secret declaration. The steps have to be skipped rather than merely starved of a token: their conditions only checked the OS and Python version, so removing the secret on its own would have left them running tokenless. Assisted-by: claude-code:claude-opus-5[1m]

Supersedes the earlier commit that made `__eq__`/`__hash__` iterative. That fixed the RecursionError but left the deeper problem in place: structural equality is not the question anyone is really asking. It reports `a + b` and `b + a` as different expressions, along with `x * 1` and `x`, `(a + b) + c` and `a + (b + c)`, and any constant spelled differently from the one that was parsed. Answering those properly is computer algebra, well outside what a syntax translator can promise, so the package should not imply an answer at all. `__eq__` now raises TypeError naming the alternative, and `__hash__` is None rather than a raising method, so a node is honestly not Hashable rather than merely failing when hashed. `eq=False` stays on the node dataclasses, now to stop the generated methods overriding the refusal rather than to stop them recursing. Comparing serializations remains well defined and is what the tests already did in most places: output is deterministic and fully parenthesized, so equal strings mean identically shaped trees. The tests that did compare nodes now compare `str()`, and the depth test for `==`/`hash` is gone with the methods. `_key()` goes with them, so a node type is back to implementing three things. Assisted-by: claude-code:claude-opus-5[1m]

Copilot

Pull request overview

This PR applies a set of correctness and release-safety fixes across formulate’s parsing/AST/serialization pipeline, along with accompanying tests and documentation updates. The headline behavioral change is emitting valid NumExpr for dotted ROOT branch names by encoding unsupported characters.

Changes:

Encode dotted ROOT symbol names when rendering to NumExpr (e.g. branch.leaf → branch_2e_leaf) and document the evaluation implications.
Tighten ROOT grammar to only allow : (multi-output) at the top level and add targeted parse-error suggestions/tests.
Improve correctness and release safety: treat overflowing float literals as the inf constant, remove misleading AST equality/hash behavior, fix constant-value comparisons, and add CD gating + packaging smoke tests.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_root_semantics.py	Adds tests for dotted-name behavior across backends and for rejecting nested multi-output `:` usage.
tests/test_identifiers.py	Adds tests ensuring NumExpr `complex()` is not translated to Python and remains NumExpr-only.
tests/test_failures.py	Extends ROOT parse suggestions for nested `:` and tightens fuzz-test assumptions.
tests/test_cycle.py	Adds `branch.leaf` to cycle/round-trip coverage.
tests/test_constants.py	Switches constant comparisons to relative tolerance with `atol=0` to avoid vacuous passes for tiny constants.
tests/test_comprehensive_features.py	Adds tests for overflowing literals → `inf` constant and updates comparisons to use canonical `str()` instead of `==`.
tests/test_ast_properties.py	Updates AST property tests to reflect non-comparable/non-hashable nodes; shifts equality assertions to `str()` comparisons.
src/formulate/toast.py	Ensures overflowing float literals become `Symbol("inf")` instead of rendering invalid bare `inf`.
src/formulate/resources/root_grammar.lark	Moves `:` handling to a top-level `outputs` rule to prevent ambiguous nesting.
src/formulate/identifiers.py	Removes Python mapping for NumExpr `complex()` (and clarifies rationale in comments).
src/formulate/exceptions.py	Adds ROOT parse-error suggestion for lone `:` while avoiding false positives on `::` namespaces.
src/formulate/AST.py	Adds name-encoding machinery for NumExpr output and makes nodes explicitly uncomparable/unhashable; enforces `eq=False` on node dataclasses; adds base `__slots__`.
src/formulate/_traversal.py	Clarifies why `type(item) is tuple` is intentional (and silences pylint complaint).
pyproject.toml	Moves coverage configuration into `pyproject.toml` and adjusts pylint configuration for module naming and protected members.
noxfile.py	Clears `dist/` before builds to avoid stale artifacts being picked up by globbing.
docs/guide/issues.rst	Documents dotted-name encoding, NumExpr `complex()` limitations for Python, and `inf` literal behavior.
docs/guide/expressions.rst	Documents top-level-only `:` semantics and removes the Python mapping for `complex`.
AGENTS.md	Updates contributor guidance to reflect coverage behavior and AST equality/hash policy.
.github/workflows/ci.yml	Makes CI reusable via `workflow_call` and tries to skip Codecov uploads when invoked from CD.
.github/workflows/cd.yml	Gates deploy on CI test matrix and adds smoke tests validating built wheel/sdist install + test.
.coveragerc	Removes legacy coverage configuration file now superseded by `pyproject.toml`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

A PR review read `github.event_name != 'release'` as a bug and suggested `== 'workflow_call'` instead. It is the other way round: `github` in a called workflow holds the caller's values, and `event_name` is never `workflow_call`, so that rewrite matches nothing and would let the steps run everywhere -- which is the failure the review was trying to prevent. See actions/runner#3146. The behaviour is counterintuitive enough to have caught a reviewer, so note it where the next person will be tempted. No change in logic. Assisted-by: claude-code:claude-opus-5[1m]

ariostas added 12 commits July 31, 2026 08:31

ariostas changed the title ~~Fix findings from a codebase review~~ fix: a few more findings from a codebase review Jul 31, 2026

ariostas added 2 commits July 31, 2026 10:15

ariostas requested a review from Copilot July 31, 2026 14:55

Copilot started reviewing on behalf of ariostas July 31, 2026 14:56 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Comment thread .github/workflows/ci.yml Outdated

ariostas merged commit 6046d7d into main Jul 31, 2026
21 checks passed

ariostas deleted the fix/review-findings branch July 31, 2026 15:10

Navigation Menu

Uh oh!

fix: a few more findings from a codebase review - #96

fix: a few more findings from a codebase review#96
ariostas merged 15 commits into
mainfrom
fix/review-findings

ariostas commented Jul 31, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ariostas commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Correctness

Release safety

Housekeeping

Deliberately not addressed

Uh oh!

codecov Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ariostas commented Jul 31, 2026 •

edited

Loading

codecov Bot commented Jul 31, 2026 •

edited

Loading