Implement the SciMLStructures interface for ArrayPartition - #643
Conversation
SciMLStructures opts every numeric array into the interface and canonicalizes it with `vec`. That is right for a contiguous array, but an `ArrayPartition` is already an `AbstractVector`, so `vec` returns it unchanged and the buffer comes back unflattened. The generic `repack` then errors, since an `ArrayPartition` cannot be rebuilt by a trivial array constructor, and its message asks for exactly this. Implements `canonicalize`, `replace` and `replace!` for the `Tunable` portion, laying the partitions out one after another so the flat buffer matches `collect(p)`. The buffer is a copy, so `aliases` is `false`: the partitions are separate arrays and no flat view over them exists. It is built from the first partition rather than as a `Vector` so that an exotic backing type stays itself. `replace` takes `new_values::AbstractArray` rather than leaving it untyped, which would be ambiguous with the generic `replace(::Tunable, ::AbstractArray, ::AbstractArray)`.
| isempty(p.x) && throw( | ||
| ArgumentError("cannot canonicalize an `ArrayPartition` with no partitions") | ||
| ) | ||
| return copyto!(similar(first(p.x), length(p)), p) |
There was a problem hiding this comment.
It cannot assume that it all matches p.x[1] form.
Review feedback: sizing the buffer from `p.x[1]` assumed every partition
shares that one's array and element type. `ArrayPartition([1, 2], [3.0,
4.5])` gave a `Vector{Int}` buffer and truncated the second partition.
`reduce(vcat, p.x)` promotes across both, and keeps a uniformly static
partitioning as an `MVector` where sizing from the first partition
dropped it to a `Vector`.
`reduce(vcat, (x,))` returns `x` itself, so a single partition is copied
rather than handing back a buffer that aliases `p` while `canonicalize`
reports `aliases = false`.
| MonteCarloMeasurements = "0987c9cc-fe09-11e8-30f0-b96dd679fdca" | ||
| Polyester = "f517fe37-dbe3-4b94-8317-1923a5111588" | ||
| ReverseDiff = "37e2e3b7-166d-5795-8a7a-e32c996b4267" | ||
| SciMLStructures = "53ae85a6-f571-4167-b2af-e1d143709226" |
There was a problem hiding this comment.
just make a dep it's small
Review feedback: the package is small, so the interface moves from `ext/RecursiveArrayToolsSciMLStructuresExt.jl` into `src/scimlstructures.jl` and SciMLStructures becomes a direct dependency.
|
@ChrisRackauckas both addressed.
SciMLStructures is now a direct dependency and the code moved from Tests added for mixed element types, mixed array types and the single-partition aliasing, 53 assertions in total, Core group green locally. Description updated too, it still described the old buffer construction. |
Runic's format check flagged it; the file picked it up when the code moved out of the extension module.
|
@ChrisRackauckas sorry for the multiple pings. Fixed the Runic failure here, could you trigger CI again please? |
2 participants
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
ArrayPartition, insrc/scimlstructures.jlwith SciMLStructures as a direct dependency.vec. That is right for a contiguous array, but anArrayPartitionis already anAbstractVector, sovecreturns it unchanged and the buffer comes back unflattened. The genericrepackthen errors, and its message asks for exactly this: "Please define the SciMLStructures interface for this type."canonicalize,replaceandreplace!for theTunableportion, laying the partitions out one after another so the flat buffer matchescollect(p).reduce(vcat, p.x). The partitions need not share an array type or an element type, so taking the buffer's type from one of them is wrong:ArrayPartition([1, 2], [3.0, 4.5])would give aVector{Int}and truncate. Concatenating promotes across both, and keeps a uniformly static partitioning as anMVector.reduce(vcat, (x,))returnsxitself, so a single partition is copied. Otherwise the buffer would aliaspwhilecanonicalizereportsaliases = false. The test asserts this by mutating the buffer and checking the partitions are untouched.replacetakesnew_values::AbstractArrayrather than leaving it untyped, which would be ambiguous with the genericreplace(::Tunable, ::AbstractArray, ::AbstractArray).replace!writing into the same partition arrays rather than replacing them, uneven, single and empty partitions, mixed element and array types, integer element types, and the five non-tunable portions reporting absent.ArrayPartitionright-hand side needs a flat buffer and a way back. Related:isscimlstructureopts in numeric arrays whoserepackthen errors SciMLStructures.jl#79.