Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3108 +/- ##
==========================================
- Coverage 84.34% 83.59% -0.76%
==========================================
Files 260 260
Lines 59083 59138 +55
Branches 4996 5001 +5
==========================================
- Hits 49836 49437 -399
- Misses 8416 8830 +414
- Partials 831 871 +40
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:
|
| f = alias or self.target.c0 | ||
| neighborhood = f.grid.distributor.neighborhood | ||
|
|
||
| # The ranks are taken from the `nb` provided at runtime by the (possibly |
There was a problem hiding this comment.
are u sure that the proper fix ain't in _arg_values? _arg_values is where u should pull the override
see for example MPIMsg._arg_values
| # The ranks are taken from the `nb` provided at runtime by the (possibly | ||
| # overriding) Grid or SubDomain, as `f` may be a compiler-generated Array | ||
| # still attached to the Grid the Operator was built with | ||
| nb = f.grid.distributor._obj_neighborhood |
There was a problem hiding this comment.
I still don't get why not args.grid, the grid is supposed to be unique to the operator.
There was a problem hiding this comment.
Because f.grid may be a subdomain with it's own distributor and _obj_neighborhood which differs from the base grid.
| # The ranks are taken from the `nb` provided at runtime by the (possibly | ||
| # overriding) Grid or SubDomain, as `f` may be a compiler-generated Array | ||
| # still attached to the Grid the Operator was built with | ||
| nb = f.grid.distributor._obj_neighborhood |
There was a problem hiding this comment.
this whole thing looks like it's doing what nb._arg_defaults should be doing by hand
There was a problem hiding this comment.
seconded (related to what I wrote above)
| # `alias` may be a compiler-generated Array with no override | ||
| name = alias.grid.distributor._obj_neighborhood.name | ||
| neighborhood = None | ||
| for i in getattr(args, 'discretizations', ()): |
There was a problem hiding this comment.
why do u need getattr?
There was a problem hiding this comment.
Good spot, it doesn't need it
| return alias | ||
|
|
||
| def _arg_values(self, args=None, **kwargs): | ||
| return self._arg_defaults(alias=self._arg_alias(**kwargs), args=args) |
There was a problem hiding this comment.
That's the oposite of the alias logic. alias is the object used to build the operator it should be new._arg_defaults(alias=self, ..
There was a problem hiding this comment.
Wait, but that's not how it's used here in current main. We have alias = kwargs[f.name] above, so alias here is coming from the runtime value, not the build time value. This is essentially just refactoring of what was already here to avoid duplication in MPIMsgEnriched
There was a problem hiding this comment.
Then it's incorrect in main too
| neighborhood = i.distributor.neighborhood | ||
| break | ||
|
|
||
| return self._arg_defaults(alias=alias, args=args, neighborhood=neighborhood) |
There was a problem hiding this comment.
same should new._arg_defaults(alias=self probably should rename the new _arg_alias as _arg_new or something
3b60c57 to
45ea066
Compare
Previous merge broke some decoupler tests in PRO due to compiler-generated arrays taking their neighbouring ranks from the
Gridwith which they were built rather than the one supplied at apply time. This specifically appeared indiag2/overlap2/fullmode.