Accept vectors in PiecewiseSpace - #574
Open
jishnub wants to merge 14 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #574 +/- ##
==========================================
- Coverage 75.20% 74.16% -1.04%
==========================================
Files 79 81 +2
Lines 8445 8644 +199
==========================================
+ Hits 6351 6411 +60
- Misses 2094 2233 +139 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The merge commit 263f875 left a trailing '=' after the uuid value, making Project.toml invalid TOML. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZeBMyvW7qYiDfC7Tkb1vT
BlockInterlacer.blocks holds one blocklength sequence per component, as BlockInterlacer(sp::DirectSumSpace) shows. A bare Space is a single component, so it needs a 1-element outer collection; without the tuple, each individual block length was mistaken for a whole component. For an infinite-dimensional space this gave IteratorSize == SizeUnknown() instead of IsInfinite(), and iteration threw CanonicalIndexError from _setindex, since [zero(i) for i in 1:length(blocks)] over an infinite range is a lazy immutable vector. The tuple form also keeps the TrivialInterlacer fast path matching. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZeBMyvW7qYiDfC7Tkb1vT
Regression test for interlacer(sp::Space) wrapping blocklengths in a 1-element collection. Covers both a finite space and an infinite- dimensional one, where the unwrapped version gave SizeUnknown() instead of IsInfinite() and threw CanonicalIndexError on iteration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZeBMyvW7qYiDfC7Tkb1vT
Splitting the `for TYP in (:SumSpace,:PiecewiseSpace)` @eval loop into separate definitions revealed that the SumSpace variants were never tested: they had previously shared source lines with the PiecewiseSpace ones, so the PiecewiseSpace tests made them look covered. Covers the two-argument constructors, the AbstractArray constructor, canonicalspace, and the ConstantSpace{AnyDomain} domain-inheriting hack, plus PiecewiseSpace(::Set). PointSpace has no hash matching its ==, so equal-valued Sets of spaces compare unequal; the Set test canonicalizes rather than comparing sets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZeBMyvW7qYiDfC7Tkb1vT
Once the pieces of a `PiecewiseSpace` may be stored in a `Vector`, the
operator side has to cope with a `Diagonal` whose diagonal is a `Vector`
rather than an `SVector`. Two places silently assumed the latter:
- `convert(::Type{Operator{T}}, ::InterlaceOperator)` mapped over the
`Diagonal` itself. `map` over a `Diagonal` also applies the function
to the structural zeros, and the result of converting a
`ZeroOperator{T,UnsetSpace,UnsetSpace}` cannot be stored back in a
`Diagonal` of concrete operators. Map over the diagonal instead.
- `blockbanded_interlace_convert!` relied on `map` densifying the
`Diagonal`, which it does for an `SVector` diagonal (giving an
`SMatrix`) but not for a `Vector` one. The preserved `Diagonal` then
handed back plain zero `Matrix`es off the diagonal, whose
`blocksize` is `(1,1)`, so the guard on the block index passed only
for the first block and the rest were misplaced. Fill in the
structural zeros explicitly.
`perm` also had methods for two tuples and for two vectors but not for a
mix, which `maxspace_rule` needs as soon as a tuple-backed and a
vector-backed space meet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M8nc53g1jq2WxsH6S4BS7Q
Constructing a `PiecewiseSpace` funnelled every input through
`union(spacesin)`, which always returns a `Vector`, so a tuple of spaces
became vector-backed. That flipped every construction path, not just the
ones that are genuinely of runtime length:
- `PiecewiseSpace(Chebyshev.(components(d)))` for `d = Segment ∪
Segment`, `A ∪ B`, `PiecewiseSpace(A, B)` and
`Space(UnionDomain(a, b))` all start from a tuple.
- only `PiecewiseSegment`, and hence `ContinuousSpace`, has a genuinely
runtime number of pieces.
Collapsing a tuple into a vector costs more inference than it gains.
`PiecewiseSpace` exists to hold a different space per piece, and a vector
of mixed pieces has an abstract eltype, so `component(f, k)` inferred as
`Fun` rather than as a small union of concrete `Fun` types.
Keep the container the caller supplied instead: a tuple stays a tuple, so
the number of pieces and the type of each stay in the type, and a vector
stays a vector, so `canonicalspace(::ContinuousSpace)` is concretely
inferred. Duplicates are rare -- `union(::Space, ::Space)` already
short-circuits equal spaces before a `PiecewiseSpace` is built -- so a
tuple falls back to the vector `union` returns only when a piece was
actually removed.
This also keeps `SV` unchanged for the tuple-backed spaces that downstream
packages dispatch on.
Two consequences of the two storages now coexisting:
- `spacescompatible` for direct sums required both arguments to have the
same type, so a tuple-backed and a vector-backed `PiecewiseSpace`
would silently compare as incompatible. Compare the pieces instead,
after checking the two are the same kind of sum.
- `promote_rule` for a `Fun` over a `PiecewiseSpace` read the per-piece
types out of `SV.parameters`, which for a `Vector` yields its eltype
and its dimension. Give the vector case its own method.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M8nc53g1jq2WxsH6S4BS7Q
`conversion_rule`, `maxspace_rule` and `union_rule` compared the canonicalised pieces of the two spaces with `cs1 == cs2`. Now that the pieces of a `PiecewiseSpace` may be stored either in a tuple or in a vector, the two sides may use different containers, and a tuple never compares equal to a vector however its elements compare. The comparison was therefore false for two spaces with matching pieces, the rule fell through to the branch that sorts the first space, and `perm` returned the identity permutation, so the rule called itself with its arguments unchanged and recursed until the stack overflowed. The mixed pair is ordinary rather than exotic: `Space(UnionDomain(a, b, c))` is tuple-backed while a range space built with `PiecewiseSpace(map(rangespace, B))` is vector-backed, and solving an ODE on three pieces brings the two together. Two pieces were spared only because the `length(S1) == length(S2) == 2` branch above caught them first. Compare the pieces one by one instead, and refuse to recurse on a permutation that reorders nothing so that a similar mistake cannot hang again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHnCGhPiCzGdqz98Z6pNVZ
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
1 participant
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.
Currently,
PiecewiseSpaceconverts vectors toTuples, which is intrinsically type-unstable. Since it is often used in contexts where the number of terms isn't known at compile time (e.g.roots), the way to improve type-inference is to allow vector arguments.Along with JuliaArrays/FillArrays.jl#291, this makes the following type-inferred as a
Union: