(Towards #3398) move LFRicRedundantComputationTrans and update to use kwargs.#3417
(Towards #3398) move LFRicRedundantComputationTrans and update to use kwargs.#3417arporter wants to merge 11 commits into
Conversation
|
Test failure is the treesitter one that #3408 fixes. |
arporter
added
the
Blocked
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3417 +/- ##
=======================================
Coverage 99.96% 99.96%
=======================================
Files 389 390 +1
Lines 54598 54616 +18
=======================================
+ Hits 54579 54597 +18
Misses 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
arporter
removed
the
Blocked
|
A fairly straightforward PR that just moves LFRicRedundantComputationTrans to the correct location and updates it to use kwargs. One for @LonelyCat124 @sergisiso or @hiker. |
There was a problem hiding this comment.
Hi @arporter - few things to sort out, mostly in tests/formatting but I think there are a couple of errors in the kwargs update also.
Edit: Also can you double check if this is used in lfric_apps/core scripts and make sure they're updated with the new import location (conditionally with importError probably).
| import RaisePSyIR2LFRicAlgTrans | ||
| from psyclone.domain.lfric.transformations.lfric_loop_fuse_trans \ | ||
| import LFRicLoopFuseTrans | ||
| from psyclone.domain.lfric.transformations.lfric_redundant_computation_trans \ |
There was a problem hiding this comment.
Use parentheses here instead of \. You don't need to change the whole file unless you want.
| validity checks. | ||
| :param options: a dictionary with options for transformations. | ||
| :type options: Optional[Dict[str, Any]] | ||
| :param int options["depth"]: the depth of the stencil if the value |
There was a problem hiding this comment.
You don't need to keep the options declarations in the docstring now.
There was a problem hiding this comment.
Once thats done I'll check the generated docs are correct.
| # TODO #2668: Deprecate options dictionary. | ||
| depth = options.get("depth") | ||
| else: | ||
| depth = self.get_option("depth", **kwargs) |
There was a problem hiding this comment.
You don't need the else block for the apply method - we just have the depth kwarg value. Do we have a test for this behaviour as I think the current implementation should fail that test as self.get_option("depth", **kwargs) should always return None here I think.
| rc_trans = LFRicRedundantComputationTrans() | ||
| loop = schedule.children[4] | ||
| with pytest.raises(TransformationError) as excinfo: | ||
| rc_trans.apply(loop, {"depth": 0}) |
There was a problem hiding this comment.
Update the test to use keywords.
There was a problem hiding this comment.
Keep one test (or duplicate one test) with kwargs and add a TODO to remove it.
| rc_trans = LFRicRedundantComputationTrans() | ||
| loop = schedule.children[4] | ||
| with pytest.raises(TransformationError) as excinfo: | ||
| rc_trans.apply(loop, {"depth": 1}) |
| schedule = invoke.schedule | ||
| rc_trans = LFRicRedundantComputationTrans() | ||
| loop = schedule.children[4] | ||
| rc_trans.apply(loop, {"depth": 3}) |
| # there are 3 halo exchange calls | ||
| index = 3 | ||
| loop = schedule.children[index] | ||
| rc_trans.apply(loop, {"depth": 3}) |
There was a problem hiding this comment.
Also lines 324, 370, 424, 473, 561, 614, 728, 783, 792, 809, 830, 853, 859, 866, 902, 910, 1014, 1045-1046, 1052, 1062, 1087, 1094, 1118, 1126, 1153, 1160, 1168, 1229
|
Looking at the ITs failures it lokos like this was handled previously by the Dynamo compatability layer in transformations.py - so either we can update that or we can update lfric_apps/core scripts (the latter is probably preferable at this point). |
|
@arporter @LonelyCat124 I have the new branches that force push to our lfric_apps almost ready, let me do the change and give you a hash to try, otherwise it will conflict with my changes. I will look at this tomorrow. |
|
OK, thanks Sergi. |
|
PS: Another branch is already bringing this update, so not hash update should be necessary once this is brought to master |
3 participants
An initial step before updating the options accepted by this transformation.