fix[next]: support module-qualified offsets in unstructured shifts - #2724
Closed
havogt wants to merge 1 commit into
Closed
fix[next]: support module-qualified offsets in unstructured shifts#2724havogt wants to merge 1 commit into
havogt wants to merge 1 commit into
Conversation
`field(mod.Off)` raised `FieldOperatorLoweringError: Unexpected shift arguments!` because the `field(Off)` arm of `_visit_shift` matched `foast.Name` only. Extend it to `foast.Attribute`. `ts.OffsetType` carries no name, so the lowering takes the offset tag from the source-level identifier. The Cartesian arm does not have this problem: it matches on `ts.DimensionType`, which carries the `Dimension` itself, so it is node-shape agnostic already. Also add tests for module-qualified Cartesian and staggered shifts, which work but were uncovered. Known remaining gap: an offset renamed on import (`from mod import V2E as W2E`) still lowers to the wrong tag, since `attr`/`id` is the source-level name rather than `FieldOffset.value`. Embedded resolves via `FieldOffset.value` and disagrees. Fixing that means putting the name in `OffsetType`/`ConnectivityType`.
2 tasks
Contributor
Author
|
Closing in favor of #2730 |
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.
Description
field(mod.Off)raisedFieldOperatorLoweringError: Unexpected shift arguments!, whileneighbor_sum(..., axis=mod.LocalDim)worked. Thefield(Off)arm of_visit_shiftmatchedfoast.Nameonly, so a module-qualified offset (afoast.Attribute) fell through tocase _. Extended to also matchfoast.Attribute.Only the unstructured arm was affected. The Cartesian arm matches on
left=foast.LocatedNode(type=ts.DimensionType(dim=...)), which is node-shape agnostic and carries theDimensionin the type — sofield(mod.Dim + 1)already worked. (It was broken the same way until #2667, which changed that pattern fromfoast.Nameto the type-driven form as a side effect of the staggered-shift work.)The asymmetry is that
ts.OffsetTypecarries onlysource/target, not the connectivity name, so the unstructured lowering has to fall back on the source-level identifier and therefore must match node shape.Tests
Added to
test_import_from_mod.py, all across the backend matrix:test_import_dims_module_cartesian_shift—a(cases.IDim + 1)test_import_dims_module_staggered_shift—a(cases.IHalfDim + 0.5)test_import_offset_module_unstructured_shift—neighbor_sum(a(cases.V2E), axis=cases.V2EDim)The first two pass without the fix; the third is the regression test.
Known remaining gap (not fixed here)
Because the tag comes from the identifier, an offset renamed on import lowers to the wrong tag:
Embedded resolves through
FieldOffset.value(fbuiltins.py:485-488), the compiled path through the AST identifier. This dates back to the first FOAST shift lowering (#625, 2022-01-26) and became observable when embedded remap landed (#1309). Fixing it properly means carrying the name inOffsetType/ConnectivityType(cf. the existingTODO(havogt)onOffsetType), which would also make this arm type-driven and let the shape match go away entirely. Left for a follow-up since it touchesOffsetTypeequality.Requirements