Skip to content

Accept any AbstractVector or AbstractMatrix of block properties - #332

Merged
cmhamel merged 1 commit into
mainfrom
fields/property-field-inputs
Aug 7, 2026
Merged

Accept any AbstractVector or AbstractMatrix of block properties#332
cmhamel merged 1 commit into
mainfrom
fields/property-field-inputs

Conversation

@lxmota

@lxmota lxmota commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #331.

create_properties is part of the physics interface, and an implementation that returns an SVector — the natural thing to return, and what FEC's own defaults returned until #331 — 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 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 AbstractVector of blocks and normalizes each entry to a dense array we own:

  • a vector-like block is constant across the block,
  • a matrix-like block is one column per element,
  • the two still mix freely.

Anything else now raises an ArgumentError naming the offending type and both accepted shapes, instead of an @assert that could only fire after dispatch had already succeeded — by construction it was unreachable, since a rank-3 array never matched Vector{<:Array{T}} in the first place.

Two fixes fall out of normalizing up front:

  • Element types are promoted across blocks rather than being required to match, so [[1, 2], [3.5, 4.5]] works.
  • The field no longer aliases the caller's arrays. 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_properties widens from Vector to AbstractVector{<:Number} for the same reason, so a single shared SVector of properties across all blocks works too.

Testing

  • FEC: 51913 pass (+16, all new).
  • Carina: 810/810 green against this branch, on both the explicit and quasi-static GPU paths.

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.

`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>
@lxmota
lxmota requested a review from cmhamel August 6, 2026 20:33
@lxmota

lxmota commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

There seem to be a GitHub actions outage that is causing these failures.

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.91%. Comparing base (0d3ab53) to head (c8f2c38).
⚠️ Report is 11 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cmhamel
cmhamel merged commit 125097b into main Aug 7, 2026
10 of 21 checks passed
@cmhamel
cmhamel deleted the fields/property-field-inputs branch August 7, 2026 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants