Repository navigation
feat: multi-field API — set!(fun, ϕ), op!, and grid/field compatibili… - #102
Conversation
…ty checks Expand the package for contexts that combine several fields at once. - `set!` now takes the function first: `set!(fun, ϕ)`. This is breaking (0.1 → 0.2) and makes the `do`-block form read naturally. Every call site in src, ext, test, docs, examples, benchmarks and the README is updated; the multi-device method in the MDLA extension becomes `set!(fun, x, P)` for consistency. - New `op!(fun, ϕ, ϕs...; check=true)`: pointwise in-place update of `ϕ` from the cell coordinates and the current values of `ϕ` and any number of other fields, on `Field`s and forest fields alike. - Compatibility checks for multi-field functions: grid `==` (same discretization, device ignored), `same_layout`, `compatible`, `check_compatible`, `check_layout`, with a first-mismatch property walk, an identity fast path, an out-of-line throw, and regrid-currency checks for forest fields. Variadic forms recurse over the argument tuple and measure 0 B under `--check-bounds=yes`. - Unified leaf interface: `nleaves`, `leaf_grid`, `block`, `_block_array` and `_require_current` on `CartesianGrid`/`Field`, so per-block sweeps can be written once against `AbstractGrid`/`AbstractField`. - Docstrings for `set!`, `op!` and grid `==`; new names exported; benchmark compat bumped to 0.2. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kylebeggs
left a comment
kylebeggs
left a comment
There was a problem hiding this comment.
Overall fantastic additions. Just my one question on the AbstractGrid style
| for i in 1:nleaves(f.grid) | ||
| set!(block(f, i), fun) | ||
| function set!(f::F, ϕ::AbstractBlockField) where {F} | ||
| for i in 1:nleaves(AbstractGrid(ϕ)) |
There was a problem hiding this comment.
why do we construct AbstractGrid here?
There was a problem hiding this comment.
It's a getter. This is a style I've used quite a bit across many packages and it pops up occasionally around the Julia ecosystem. We can change it if you want, but I still think we need a getter so as to be generic across different types.
There was a problem hiding this comment.
agreed we need a getter, its a style question mainly - typically i would use lowercase with _ for this and not include "Abstract" i only use "Abstract" for abstract type. not necessarily wrong but not what im used to here. A little hesitant to move forward with this style unless you have a good reason
There was a problem hiding this comment.
The idea is that it's getting the (unique) object of type AbstractGrid that is associated with the field. If the only such object was necessarily a CartesianGrid, it would be CartesianGrid. Would you prefer getgrid or something like that?
There was a problem hiding this comment.
getgrid (or even just grid) is what im used to so im biased towards that. @joshday thoughts?
There was a problem hiding this comment.
i want to try to stay open minded with these things..
|
btw, neither me nor claude can figure out why this benchmark job fails. Cannot reproduce locally. |
|
I've lifted the WIP status: should be ok to be merged after we resolve issues and if you are willing. There's more I'd like to do but I think this is enough for this PR. |
|
well the benchmark Ci runs but fails for a different reason. progress i guess lol |
f719c99 to
a43e092
Compare
|
Changed the getter to Btw we also probably need a smaller test suite that we can easily run, the full test suite takes 45 minutes somehow. |
a43e092 to
2c56d2a
Compare
2c56d2a to
d989b79
Compare
|
I had claude comment out the minimum of tests to get 1.13 to pass despite whatever is happening with enzyme. I did not seriously sanity check it, but it was a fairly small change, so should be ok |
|
ok im just gonna merge this and worry about the benchmarking later. I confirmed no regressions locally |
…ty checks
Expand the package for contexts that combine several fields at once.
set!now takes the function first:set!(fun, ϕ). This is breaking (0.1 → 0.2) and makes thedo-block form read naturally. Every call site in src, ext, test, docs, examples, benchmarks and the README is updated; the multi-device method in the MDLA extension becomesset!(fun, x, P)for consistency.op!(fun, ϕ, ϕs...; check=true): pointwise in-place update ofϕfrom the cell coordinates and the current values ofϕand any number of other fields, onFields and forest fields alike.==(same discretization, device ignored),same_layout,compatible,check_compatible,check_layout, with a first-mismatch property walk, an identity fast path, an out-of-line throw, and regrid-currency checks for forest fields. Variadic forms recurse over the argument tuple and measure 0 B under--check-bounds=yes.nleaves,leaf_grid,block,_block_arrayand_require_currentonCartesianGrid/Field, so per-block sweeps can be written once againstAbstractGrid/AbstractField.set!,op!and grid==; new names exported; benchmark compat bumped to 0.2.