Accept any AbstractVector or AbstractMatrix of block properties - #332
Merged
Conversation
`create_properties` is part of the physics interface, and an implementation
that returns an `SVector` -- which is the natural thing to return, and what
FEC's own defaults returned until recently -- could not build a
`PropertyField`. `Vector{SVector{N,T}}` matches neither
`PropertyField(::Vector{<:Vector{T}})` nor `PropertyField(::Vector{<:Array{T}})`,
so the failure was a bare MethodError naming two internal constructors, with
nothing to say what the caller should have passed. Carina hit exactly this.
Collapse the two constructors into one that takes any AbstractVector of
blocks, normalizing each to a dense array we own. A vector-like block is
constant across the block, a matrix-like block is one column per element, and
the two still mix freely. Anything else now raises an ArgumentError that names
the offending type and both accepted shapes, rather than an @Assert that could
only fire after dispatch had already succeeded.
Two things fall out of normalizing up front. Element types are promoted across
blocks instead of being required to match, and the field no longer aliases the
caller's arrays -- `reduce(vcat, arrs)` returned the input itself for a
single-block mesh, so mutating the array you passed in silently rewrote the
properties.
`_setup_properties` widens from `Vector` to `AbstractVector{<:Number}` for the
same reason, so a single shared `SVector` of properties works too.
Tests cover static arrays, mixed static and element-level blocks, eltype
promotion, the rejection paths and the aliasing fix. FEC 51913 tests pass;
Carina's suite is green at 810/810 against this branch.
Signed-off-by: Alejandro Mota <amota@sandia.gov>
Contributor
Author
|
There seem to be a GitHub actions outage that is causing these failures. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
==========================================
- Coverage 70.93% 70.91% -0.02%
==========================================
Files 54 54
Lines 6293 6289 -4
==========================================
- Hits 4464 4460 -4
Misses 1829 1829 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cmhamel
approved these changes
Aug 7, 2026
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.
Follow-up to #331.
create_propertiesis part of the physics interface, and an implementation that returns anSVector— the natural thing to return, and what FEC's own defaults returned until #331 — could not build aPropertyField.Vector{SVector{N,T}}matches neitherPropertyField(::Vector{<:Vector{T}})norPropertyField(::Vector{<:Array{T}}), so the failure was a bareMethodErrornaming two internal constructors with nothing to say what the caller should have passed instead. Carina hit exactly this and had to change its own return type to work around it.This collapses the two constructors into one that takes any
AbstractVectorof blocks and normalizes each entry to a dense array we own:Anything else now raises an
ArgumentErrornaming the offending type and both accepted shapes, instead of an@assertthat could only fire after dispatch had already succeeded — by construction it was unreachable, since a rank-3 array never matchedVector{<:Array{T}}in the first place.Two fixes fall out of normalizing up front:
[[1, 2], [3.5, 4.5]]works.reduce(vcat, arrs)returns the input itself for a single-block mesh, so mutating the array you passed in silently rewrote the material properties. There is a regression test for this._setup_propertieswidens fromVectortoAbstractVector{<:Number}for the same reason, so a single sharedSVectorof properties across all blocks works too.Testing
New tests cover static-array blocks, static mixed with element-level, eltype promotion, all three rejection paths, and the aliasing fix.
Note on CI
GitHub Actions has been in a major outage since 15:22Z today, and it never picked up the last push to #331 either. If the checks here stay empty that is why, not the branch — both suites above were run locally on Julia 1.12.6 (CPU) plus gfx1102 for the Carina GPU paths.