Add a BroadcastStyle for AbstractFill - #385
Open
jishnub wants to merge 46 commits into
Open
Conversation
jishnub
marked this pull request as draft
August 26, 2024 10:19
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #385 +/- ##
=======================================
Coverage 99.91% 99.92%
=======================================
Files 9 9
Lines 1238 1269 +31
=======================================
+ Hits 1237 1268 +31
Misses 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
This could also fix #394 |
jishnub
force-pushed
the
jishnub/broadcaststyle
branch
from
December 31, 2024 15:20
712a201 to
7296e78
Compare
An `AbstractFillStyle` now resolves conflicts the same way that a `DefaultArrayStyle` of the same dimension does: styles that win against `DefaultArrayStyle` (lazy, banded, block) win against a fill style too, and those that defer to it (e.g. `StaticArrayStyle`) keep deferring. Neither side has to define a rule for `FillStyle`/`ZerosStyle`. Packages that opt out of their own style for fills by forwarding to `DefaultArrayStyle` keep obtaining a fill: such `broadcasted` calls are re-dispatched on the arguments alone. Where the arguments carry a foreign style, only the style-independent rules are applied, recorded by the new `has_fill_rule` table, so the forwarding package stays in control. Also add the styleless `Ones op Ones -> Ones` rules for `*`, `/` and `\`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`x .^ k` lowers to a three-argument `literal_pow` broadcast, which none of the `DefaultArrayStyle` shapes that packages forward to us matched, so such calls materialized densely instead of obtaining a fill. Unwrap the `Ref`s so that the styleless rules apply, and record the rule in `has_fill_rule` so that arguments carrying a foreign style (e.g. an infinite fill, which `InfiniteArrays` marks as lazy) are routed to it as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The paths that packages defining their own broadcast style rely on weren't
exercised by the test suite, as nothing in it carries a foreign style. Give
the `InfiniteArrays` test helper the lazy style and the forwards to
`DefaultArrayStyle` that `InfiniteArrays` obtains through `LazyArrays`, and
test that fills, ranges and `literal_pow` keep being simplified through it,
that the unforwarded cases stay lazy, and that an array with a
dimension-agnostic style wins against a fill.
`Ones{Int}(∞) .* (1:∞)` threw along the way, as `_range_convert` rebuilds a
unit range out of its endpoints, which an infinite range can't convert. Return
a range whose eltype already matches as-is instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jishnub
marked this pull request as ready for review
July 29, 2026 15:19
Commenting out the `has_fill_rule` methods that record which rules hold whatever the style of the other argument is left the test suite passing, so nothing exercised the one shape they uniquely protect: `Zeros` absorbing an argument that is neither a fill nor something we can size or materialize. Without the fast path such a call fails to find a `similar` method rather than collapsing to `Zeros`, so test it through the `DefaultArrayStyle` entry point that packages forward to. Drop the `_range_convert` method for a `OneTo` whose eltype already matches. The diagonal `where T` makes it lose against the `OneTo` method that converts the endpoint, so it was never dispatched to, and the comment above it named the wrong methods. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Commenting each entry out in turn showed which ones a test would notice. The `+`/`-` entries for a `Zeros` against an arbitrary vector, and the `literal_pow` one, are needed whenever the other argument can't be materialized, so give the test helper an infinite vector that is neither a fill nor a range and exercise all three through the `DefaultArrayStyle` entry point. `.^` on a fill is caught by the styleless rule before a style is ever consulted, so the forwarded three-argument form needs testing on its own. The `AbstractOnes`/`AbstractOnes` entry is neither observable nor needed to resolve an ambiguity, as two fills let the caller evaluate the operation on the fill values instead, so drop it and record the criterion. The remaining two-fill entries stay: without them the one-sided entries they sit next to are ambiguous, which is what they are now documented as being for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither entry can be dropped — `Zeros`/`Zeros` and `ZerosVector`/`ZerosVector` disambiguate the one-sided entries beside them, and Aqua reports the ambiguities as soon as either goes — but nothing reached them either, as the infix tests forward through a layer that lets them be resolved before the call is made. Call the forwarded form directly, as a package would. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
For any other size, Base's `*(::AbstractArray, ::Number)` broadcasts, and the broadcast rules already produce the same result: a 306-probe sweep over fill types, element types and scalar types is unchanged. Zero-dimensional fills still need the methods, as Base's `broadcast_preserving_zero_d` mishandles what our rules return.
`Base.broadcast_preserving_zero_d` mishandles what our broadcast rules return, so `Fill(2)/2` and `2\Fill(2)` errored on a missing `similar` while `Zeros()/2` came back as a zero-dimensional `Array` wrapping a `Zeros`. Specialize both the way `*` already is. Separately, `fill_add` broadcast with `.+`, which unwraps a zero-dimensional result to the element, so `Fill(2) + fill(3)` returned an `Int` where Base returns a container. A 1416-probe sweep over the fill types, element types, scalar types and sizes changes on exactly the 120 zero-dimensional entries and nowhere else.
It assumed `Base.broadcasted` returns a lazy `Broadcasted`, which holds only while every argument type whose rules simplify eagerly is intercepted by a more specific `real`/`imag`/`conj` method — an invariant nothing enforces. Fed such an argument it wrapped a result that was already a container: `broadcast_preserving_0d(/, Zeros(), 2)` gave `Fill(Zeros())`.
They named `Zeros`/`Ones` outright, so a package overloading `broadcasted_zeros`/`broadcasted_ones` to produce its own types got a plain FillArrays type back from `real` and `imag`. The results are unchanged for every type that leaves the hooks alone — a 55-probe sweep over the fill types, element types and sizes is identical — and the new testset covers the hooks, which had none. `fillsimilar` cannot serve here: it carries the argument's eltype, with no way to ask for `real(eltype(A))`.
Finding 1 (silent wrong answer): `transpose`/`adjoint` are recursive, so a wrapper's elements are the parent's fill value with the operation applied. Reading the parent's value skipped that, and `Fill(1+2im,3)' .+ Fill(0,1,3)` came back unconjugated — but only when the other operand was itself a fill. Finding 3 (StackOverflowError): `DefaultArrayStyle` in the `_dispatch_on_fills` `Union` is unreachable in-tree, since a fill among the arguments always resolves to a fill style, but a downstream style mapping fills onto it recursed until the stack blew. Dropped; such calls now take the foreign-style branch. Finding 2 (latent): a fill-specific method may rewrite the arguments and stay lazy, as the fill-against-a-range rules do. That result already carries the caller's style, so keep it rather than rebuilding a `DefaultArrayStyle` wrapper around the original arguments. Finding 5 nit: two `ArgumentError`s lost the "Convert ... to a Vector first" hint when they were rewritten to report a size instead of the array. Finding 4: pin the structured-matrix contract with tests. Structure is lost under `*` for `Bidiagonal`/`Tridiagonal`/`SymTridiagonal`/`UpperTriangular`/`Eye` as well as `Diagonal`, and kept under the operations that have no `Zeros` rule.
Both were verified against scripts outside the suite, which left nothing in tree to catch a reintroduction. `LazyRange` is a range with a style of its own that keeps `scalar .* range` lazy — the shape where the fill-against-a-range rewrite has to survive, and the reason real ranges never reach that branch. `DeferVec` resolves fills to `DefaultArrayStyle`, which used to recurse. Against the pre-fix source these give 4 failures and 2 `StackOverflowError`s.
`_fallback_copy` collapses the fill components before handing the rest to `DefaultArrayStyle`, which may expose a rule that returns an array outright and leave nothing to `convert`, so `[1.0,2,3] .* (Ones(3) .- Ones(3))` threw. Guard it the way `broadcast_preserving_0d` already is. `isfill` recurses through any `Transpose`/`Adjoint` parent while `broadcast_getindex_value` demanded an `AbstractFill` there, so a doubly wrapped fill threw. Recurse in both. Zero-dimensional broadcasts kept their container except where `copy` ran, which left `Fill(2) .+ 1` unwrapping to the element while `Fill(2) .^ 2` stayed a `Fill`. Drop the specialization and preserve the container throughout, as every earlier release did. The `+`/`-` rules against `Zeros` return the other argument itself, so attaching them to the operation made them pre-empt a style that would have built a container of its own: `BlockArray .+ Zeros` handed back the very `BlockArray`, and mutating the result mutated the input. Attach them to the fill styles, which win only once every other style has deferred, and add `fill_rule` so the calls packages forward still reach them. `_copy_fill` recomputed the fill value in each of `_iszeros` and `_isones`, evaluating the broadcasted function three times over `Zeros`/`Ones`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`SymTridiagonal` only began preserving its structure under broadcasting in
v1.12: on the LTS even `S .+ S` and `S .* 2` materialize, with no `FillArrays`
involved at all. The style resolves as intended there — `Zeros` still defers to
`StructuredMatrixStyle{SymTridiagonal}` — so there is simply no structure for
`similar` to keep. Expect what Base itself produces rather than dropping the
case, so the shape stays covered on every supported version.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It was introduced to hold the hand-written conflict rules against `StructuredMatrixStyle` and `FillStyle`. Resolving those generically through `_fillstyle_result` left it with a single merge rule keeping it alive when a `Zeros` meets another `Zeros`, and nothing at all reading the distinction: the absorption rules match `AbstractZeros` in argument position before any style is consulted, and `_copy_fill` decides from the value, as it must, since `exp.(Zeros(3))` is `Ones` while `Zeros .+ Ones` already collapsed to `FillStyle`. That collapse also made it a trap to build on, firing for `Zeros .* Zeros` but not `Zeros .* Ones`. `AbstractFillStyle` stays as the handle to dispatch on, and `_fillstyle_defer` still rebuilds whatever concrete style it was handed, so a package adding one of its own keeps its identity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`(1:5) .+ 1` is `2:6`, but `(1:5) .+ Ones(5)` materialized, even though `(1:5) .* Ones(5)` has kept the range all along. Rewrite `+` and `-` against a range the way `*` is rewritten, to the fill value against the range, and let the range decide what it becomes. `Zeros` needs no rule of its own, being a fill whose value is zero, but a range is a vector and `Zeros` is a fill, so the pair overlaps its rules with neither the more specific. The two methods breaking that tie do the same rewrite, which also stops `(1:5) .+ Zeros(5)` materializing where `(1:5) .+ Fill(0.0,5)` and `(1:5) .+ 0.0` both stay a range. `fill_rule` still returns the argument untouched, as the forwarded path needs: `0 .+ OneToInf()` is an `InfUnitRange` rather than an `OneToInf`, so rewriting there would lose `Zeros .+ r ≡ r`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`imag` of a real fill is zero by the eltype, but the generic path has only the
value to go on, and `_copy_fill` will not read `Zeros` out of a runtime value,
so `imag(Fill(4))` was `Zeros{Int}()` while `imag.(Fill(4))` was `Fill(0)`.
`Ones` and `Zeros` already agreed, their value being static, as did the complex
fills, whose imaginary part is a genuine runtime quantity.
State it once for the broadcast too. Going through `broadcasted_zeros` keeps a
package customizing the hook its own type here, as the applied form already
did. Nothing needs a `has_fill_rule` entry: a forwarded call falls through to
`broadcasted(FillStyle{N}(), …)`, and the array it returns comes straight back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`imag.(Fill(4))` was made `Zeros` to agree with `imag(Fill(4))`, but the rule keys on the function, so it never fires for a call that merely wraps it: `(x -> imag(x)).(Fill(4))` stayed `Fill(0)`. Trading a disagreement between the applied and broadcast forms for one between two spellings of the same broadcast is a worse deal, and every other operation-specific rule here agrees with what the generic path would have produced anyway. `imag` cannot: it is zero by the eltype, and `_copy_fill` has only the value, which `has_static_value(::Fill)` deliberately will not report on. So leave the containers differing and pin that in the tests instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Base.broadcast_preserving_zero_d` re-wraps a zero-dimensional result on the
assumption that broadcasting unwrapped it to the element. Our rules hand back a
container, so the re-wrap nested it: `Fill(4) * 2` gave a zero-dimensional array
whose element was `Fill(8)`, and `Day(1) * Fill(2)` a `MethodError` for the
`similar` we do not define. Defining `similar` does not help — a correct one,
returning a mutable `Array`, still yields `Array{Fill{Day,0},0}`.
Extend the function instead of shielding its callers. Base reaches it from eight
operations, all of which had or needed a hand-written method here, but it is not
private to Base: `Dates` routes `Period * AbstractArray` through it, so that
enumeration can never be complete. `LinearAlgebra` extends it the same way for
adjoint and transpose vectors. This lets the zero-dimensional `*`, `/` and `\`
methods go.
`broadcast_preserving_0d` now materializes rather than copying, since its
arguments need not all be fills and instantiating is what checks the shapes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six of the twelve were redundant. The `AbstractFill` three did exactly what Base
now reaches through the extended `broadcast_preserving_zero_d`. The
`AbstractFill{<:Real}` three restated Base's own `real(::AbstractArray{<:Real})`
and `imag(::AbstractArray{<:Real}) = zero(A)`, which give the same answers for a
fill; those had been redundant all along.
What is left is the `Zeros`/`Ones` half, which is not: it routes through the
`broadcasted_zeros`/`broadcasted_ones` hooks, while the broadcast fallback names
`Zeros`/`Ones` outright and so loses a package's own fill type. Removing those
six changes seven results, all on the custom types the tests define.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_copy_fill` named `Fill`/`Zeros`/`Ones` outright, so a package customizing
`broadcasted_fill`/`broadcasted_zeros`/`broadcasted_ones` got its own type back
only from the operations with a rule of their own. Everything reaching the
generic path lost it: `exp.(Z)`, `sin.(Z)`, `real.(Z)`, `max.(Z, Z)`, the
broadcast forms of `real`/`imag`/`conj`. `fill_rule` dropped it for `Z .+ Z` and
`Z .- Z` the same way.
Dispatch on the number of arguments, since the hooks take one or two arrays, and
name the type outright only past that. Over a 29-expression sweep the hook now
fires for 27, against 7 before; the two left are fused broadcasts over three
fills, which would need an n-ary hook.
A broadcast now agrees with the applied form on a package's own types, as
`real.(TaggedZeros{ComplexF64}(4))` did not before.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hooks were fixed at one or two arrays, which is why the generic path named the type outright past that: a fused broadcast over three fills had no hook to call. Varargs have to come last, so the value and axes move ahead of them, and the two arities collapse into one method each. A rule now passes whatever arguments it has, and a package matches on the ones it cares about with a `rest...` tail. Over the 29-expression sweep the hook fires for all 29, against 27 with the arities fixed; the nested arguments reach it as the `Broadcasted` they still are, which a package can dispatch on. Nothing outside FillArrays overloads these, and they are neither exported nor documented, so the signature change breaks no known caller. The internal `_broadcasted_zeros(f, a, b)` that BandedMatrices imports is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `T` list generating the `Zeros` scaling rules named `AbstractFill{<:Number}`,
`AbstractOnes` and `AbstractRange` alongside `AbstractArray{<:Number}`. The first is
a strict subtype of it, and the other two only reach further for element types that
are not numbers, which the rules cannot serve anyway, as the comment above them says.
`real`/`imag`/`conj` on a `Zeros` or a `Ones` restated what Base already reaches
through the broadcast, which applies the same hooks. Only `imag` on a `Ones` did
more: Base sends a real element type to `zero`, which names `Zeros` outright.
`has_fill_rule` was a second table that had to agree with the rules themselves,
entry for entry. Holding the rules on `fill_rule` alone, with `nothing` for "no
rule", leaves one table and no way for the two to drift apart.
The `*`-against-a-range rules were `_shift_range` with a different operator, so
they join its loop, and `_iszeros`/`_isones` fold into their only caller, which
was evaluating their shared guard twice.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_range_convert` reached for `OneTo` and the colon constructors, which need more of
the element type than arithmetic: an integer to count up to, and a value that can be
counted from one endpoint to the other. An element type offering neither threw
instead of returning a range, so `Ones{ComplexF64}(3) .* (1:3)` and even
`Ones{Float64}(3) .* OneTo(3)` were errors.
`_rebuild_range` now dispatches on the element type and falls through to the same
`steprangelen` that `cumsum` builds, which asks only for arithmetic and which
InfiniteArrays.jl already overloads. Each rule still hands back the most structured
range its element type supports, so `OneTo` and `UnitRange` results are unchanged.
Dispatching on the element type inside a separate function, rather than narrowing the
`_range_convert` methods themselves, keeps their order intact: narrowing them to
`T<:Real` let the converting method beat the one that hands back a range whose element
type already matches, which broke `Ones{Int}(∞) .* OneToInf()`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `Zeros`-against-a-range entries in the loop exist to break a tie with the
`Zeros` rules attached to the fill styles, which only `+` and `-` have. Scaling
resolves before any style is consulted, on the styleless rule for a `Zeros`
against an `AbstractArray{<:Number}`, so its two entries never ran.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The method breaking the tie between the two one-fill methods was the only line of the branch the test suite never ran: nothing in Base reaches it, as `+` and `-` on a pair of fills have methods of their own. Call it directly, the way the neighbouring zero-dimensional tests already do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Ones .* Ones`, and the `/` and `\` forms alongside it, took the general path
to the same answer: the fill value is one, every argument has a static value,
so `_copy_fill` reaches `broadcasted_ones` on its own. `_broadcasted_ones` had
no other caller once they were gone.
`_range_convert` for an `AbstractUnitRange{T}` was a strict subtype of the
`AbstractRange{T}` method above it and returned the same thing. Its comment
described the `AbstractUnitRange` method that `_rebuild_range` has since
replaced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The methods catching the calls a package forwards through `DefaultArrayStyle` named their arguments one by one, so only a unary or binary broadcast reached the fill rules; `+.(F, F, F)` under such a style rendered an `Array`. Dispatch cannot ask whether any argument is a fill, only whether the one in a given position is, and covering the first k positions costs a method per non-empty subset of them. Taking the arguments as a vararg from the second position on covers every arity for three methods rather than four, and a fill behind two non-fills stays Base's to render. The `literal_pow` shape now matches the second of them, and is still needed to unwrap the `Ref`s that hide its fill. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither operand loses precision on the way: the fill path evaluates the operation once on the fill values and the dense path evaluates the same operation elementwise, so the two agree bit for bit. A tolerance would hide a wrong value, which is the whole of what these two lines check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Fixes #40
Fixes #58
Fixes #143
Fixes #145
Fixes #188
After this,
Often, we will have
AbstractFillreturn types returned now in broadcasting operations, and --- where possible --- ones whose values are statically known:This does not change the fact that broadcasting over a
Fillis evaluated before others in a fused broadcast:This is achieved by recursively walking through a
Broadcastedobject, and evaluating the components that produceAbstractFills eagerly.