fix(symmetry): make a symmetry tag match the buffer it is attached to - #253
Merged
Merged
Conversation
`as_symmetric` and the public `SymmetricTensor` constructor validate symmetry within a tolerance, while the cost model reads the tag they attach as exact. Both now copy one representative per orbit before attaching it, so the two readings agree. Data that is already exactly invariant is returned unchanged, keeping the existing zero-copy view semantics and `out=` layouts. Charges are unchanged: the copy is folded into the existing validation cost. `symmetrize` gains `mode=`, defaulting to the existing `reynolds-projection`, which is unchanged in result and price. The new `canonical-copy` keeps each orbit's lexicographically first entry rather than averaging, bills numel, preserves dtype, and reads only the group's generators -- so it remains available for groups too large to enumerate. `fnp.random.symmetric` takes the same argument. Reynolds symmetrization now refuses a group it cannot enumerate before charging for the projection, and names the alternative. Symmetry attachment inside the package routes through `wrap_with_derived_symmetry`; trust is anchored to `wrap_with_trusted_symmetry` alone, so `wrap_with_symmetry` is validated and charged like any other caller-supplied claim. `full` and `full_like` no longer infer symmetry from shape when `fill_value` is not a scalar.
Both delegated to `wrap_with_trusted_symmetry`, which meant an importer of either name reached the trusted code object and attached a tag to unexamined data for free -- widening the surface the previous commit set out to narrow, and reversing `wrap_with_inferred_symmetry`, which validated before it. Both now construct directly, so their exemption comes from running inside a counted op rather than from the helper they call. `matrix_transpose`, the one symmetry-propagating transform with no counted frame of its own, names the trusted wrapper explicitly and stays free. `wrap_with_trusted_symmetry` is now the only importable name that attaches a tag without a check, and the test suite pins both that fact and the behaviour of the other three.
Signed zero: `is_exactly_invariant` compared with `==`, and `-0.0 == 0.0`, so a buffer whose orbits differed only in a sign bit was taken for already-exact and passed through untouched. `copysign` reads that bit straight back out, which made it information sitting in a position the cost model prices as redundant -- reachable through registered ops alone. Zeros that disagree in sign now count as a difference, on both components of a complex value, and send the buffer down the copying path. Cached orbit map: it was marked read-only, but NumPy lets a caller re-enable the writeable flag on an array that owns its data, and an edited map silently mis-canonicalizes every later call for the same shape and group. Callers now get a view, whose base refuses the flag. `clear_cache` also drains the orbit-map cache, whose entries are array-sized rather than constant.
The orbit-map cache held one index per tensor element per distinct (shape, group action), bounded only by entry count -- so entries scaled with the tensors they described while the count did not, and `symmetrize` is a registered op, so a caller chooses the shapes. Sixty 2048x2048 maps pinned about 2 GB. It is now bounded by total bytes, with a map larger than the whole budget served but not retained so one outsized request cannot evict everything and still not fit. The module docstring claimed a tag certifies exact invariance package-wide. That is true of the ingress points and not of symmetry propagated algebraically: Reynolds sums each orbit in a fixed element order, so its own output is invariant only to about an ulp, and a symmetric matmul is the same. Those tags are sound for accounting, but the claim as written was wider than the code supports, so it now states its scope. Adds tests for the byte bound and eviction, for the `out=` consequence of copy-on-inexact, and for the one importable wrapper that still attaches a tag without checking -- stated outright rather than left out of a test file that would otherwise read as though nothing were open.
A test and two comments claimed wrap_with_trusted_symmetry was the last route to an unchecked tag. It is not, and could not be: `arr.view(SymmetricTensor)` followed by an attribute assignment mints one using only public NumPy and the class object, with no helper from this package involved, and monkeypatching the validator works too. Hardening the private helpers would close two doors in a wall that does not surround anything, so the claim is replaced with the boundary that does hold -- the server dispatches registered operations only, and none of these names is registered. Also narrows the full/full_like billing note. The rise reaches only a non-scalar fill into a shape with two or more equal-length axes, which is exactly the case that was being given a tag the data did not support; a non-scalar fill into any other shape was never tagged and is unchanged.
fnp.full and friends are typed as returning FlopscopeArray, which carries no symmetry attribute, so pyright rejected the direct reads. Assert the type the way the rest of the suite does -- which is also the stronger assertion.
The symmetry guide described `as_symmetric` as validate-then-tag, gave a single Reynolds cost for `symmetrize` and `fnp.random.symmetric`, and said the enumeration budget only degrades accounting. All three now cover what the code does: canonicalization at the tag boundary and what it means for your array, `mode="canonical-copy"` alongside the default with its own cost and dtype behaviour, and the fact that these two functions refuse rather than degrade above the budget. The cost-model reference row on symmetry tags is rewritten. It named `wrap_with_symmetry` as trusted, counted the internal call sites wrongly, and stated the exactness guarantee more widely than the code supports; it now states the guarantee's scope and where the boundary actually is.
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.
Summary
Aligns symmetry metadata with the buffers it describes, and adds a second
symmetrization mode.
Symmetry validation accepts data that is symmetric within a tolerance, while the
cost model reads the tag it grants as exact — it prices every position in an
orbit after the first as a redundant degree of freedom and does not read the
buffer again. The two readings disagree for any buffer whose orbit entries merely
agree closely:
as_symmetricand the publicSymmetricTensorconstructor now copy onerepresentative per orbit before attaching the tag, so the tag matches the buffer.
Data that is already exactly invariant is returned unchanged, which keeps the
zero-copy view semantics and
out=layouts of the common case. Charges areunchanged: the copy is folded into the existing validation cost.
Also in this change
symmetrizegainsmode=. The default,"reynolds-projection", is theexisting behaviour and is unchanged in both result and price. The new
"canonical-copy"keeps each orbit's lexicographically first entry instead ofaveraging the orbit, bills
numel(data)— the rate every other materializingcopy pays — and preserves the input dtype rather than promoting to float64. It
reads only the group's generators, so it stays available for groups too large
to enumerate.
fnp.random.symmetrictakes the same argument.Reynolds symmetrization refuses an unenumerable group before charging for
it.
symmetrizeandfnp.random.symmetricpreviously computed a projectioncost from a closed-form group order, charged it, and only then hit the
enumeration limit. They now raise
ValueErrorfirst, namingmode="canonical-copy", and the refused call costs nothing.fullandfull_likeno longer infer symmetry from shape whenfill_valueis not a scalar. The inferred group describes a constant fill; a broadcast
array fill writes distinct values into positions it would otherwise report as
redundant.
Symmetry attachment inside the package routes through
wrap_with_derived_symmetry. Trust is anchored towrap_with_trusted_symmetryalone, and the two sites that cannot inherit acounted frame name it explicitly.
Billing
as_symmetricandis_symmetricare unchanged, verified against the existingk * (7n - 1)formula and identical whether or not the copy runs.symmetrize(mode="canonical-copy")billsnumel. Reynolds pricing is untouched.A 32-operation differential against the previous release shows no other change.
One narrow rise: a non-scalar
fill_valueinto a shape with two or moreequal-length axes loses a tag its data did not support, so its results are priced
as the dense arrays they are. Any other shape was never tagged and is unchanged.
Details in the CHANGELOG.
Tests
tests/test_symmetry_canonicalization.py(77 tests) covers exact invarianceunder symmetric, cyclic, dihedral and generator-defined groups; the
representative rule; dtype preservation across float, integer, boolean and
complex; orbit-map cache keying, byte bound and eviction; and that trusted
propagation adds no per-operation cost. Full suite green.