Repository navigation
Conversation
| if isinstance(p, MPINeighborhood): | ||
| at_args.update( | ||
| MPINeighborhood(p.neighborhood)._arg_values() | ||
| p._rebuild()._arg_values() |
There was a problem hiding this comment.
p._rebuild() without any agument is also supposed to be no-op so that's worrying
There was a problem hiding this comment.
It returns a copy, built from scratch iirc. That being said, the copy should be done explicitly rather than hidden behind a _rebuild. Tweaking.
FabioLuporini
left a comment
FabioLuporini
left a comment
There was a problem hiding this comment.
looks sane, but I'd lie if I claimed I fully understood it
so corollary: there can be multiple neighbourhood objects in the gen code, in particular when Functions-on-subdomains are present?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3099 +/- ##
==========================================
- Coverage 84.51% 84.47% -0.05%
==========================================
Files 260 260
Lines 58493 58533 +40
Branches 4962 4963 +1
==========================================
+ Hits 49438 49443 +5
- Misses 8226 8258 +32
- Partials 829 832 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Full disclaimer: this was Claude cooked whilst I was working on something else. The original bug arose when trying to implement the adjoint for CPML. I still need to give it a proper review myself.
Yes, that's correct, since a function on a subdomain can have a different neighbourhood to the grid when the subdomain doesn't cross a rank edge (so you have an empty rank on the other side, for which there should be no halo exchange) |
| def _obj_neighborhood(self): | ||
| # The neighborhood is determined by the SubDomain thicknesses, hence the | ||
| # name. E.g., `nb_x0_7` for `{x: ('middle', 0, 7), y: y}` | ||
| name = ''.join(f'_{d.root.name}{d.ltkn.value}_{d.rtkn.value}' |
There was a problem hiding this comment.
why ltkn/rtkn.value instead of .name
There was a problem hiding this comment.
It's not mandatory to give subdomains unique names, although I agree that this solution is kinda inelegant
|
|
||
| # Update `nb` based on the distributor of the (possibly overriding) | ||
| # Functions using it. E.g., an override may be defined on a new Grid | ||
| for p in inputs: |
There was a problem hiding this comment.
This doesn't look right to have to go through all of the op inputs withing an _arg_values
af46c21 to
2ed4bc6
Compare
| __rkwargs__ = ('name',) | ||
|
|
||
| def __init__(self, neighborhood): | ||
| def __init__(self, neighborhood, name='nb'): |
There was a problem hiding this comment.
Maybe a better default name, since nb is used for e.g boundary size and such
There was a problem hiding this comment.
The current default name is nb though, so this is no change really
| # Use `nb` as provided by the (possibly overriding) Grid or SubDomain. | ||
| # E.g., an override may be defined on a new Grid | ||
| if self.name in kwargs.get('args', {}): | ||
| return {} |
There was a problem hiding this comment.
shouldn't it be the usual
key = kwargs.get('args', {}).get(self.name, self)
return key._arg_defaults()
There was a problem hiding this comment.
No, since it should only fire here if the override/default has not already been processed by either Grid._arg_values or AbstractSubDomain._arg_values
Fixes an issue which surfaced with
Functions onSubDomains.Issue: a Function on a
SubDomain, run under MPI, gets garbage, NaN or stale values in its halo whenever a neighbouring rank holds none of theSubDomain. Two separate causes:Codegenpicks theSubDomain's neighbourhood, which says "no neighbour" (PROC_NULL) there. At apply,MPINeighborhood._arg_valuesreplaced it with the rootGrid's. Rank 0 then posted a receive from an empty rank that sent 0 floats, and copied an uninitialisedposix_memalignbuffer into its halo.Decomposition.reshapegave the outhalo rows to the outermost rank even when that rank was empty. The defaultdata_with_halo[:] = 0sent those rows there, so rank 0's halo kept uninitialised memory, which the stencil read.