SingleColumn: Fix vectorisation of nested else-if bodies #392
Conversation
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/392/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
=======================================
Coverage 95.53% 95.53%
=======================================
Files 186 186
Lines 38974 39005 +31
=======================================
+ Hits 37233 37263 +30
- Misses 1741 1742 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Only a comment for now, but changes look very good. I think your solution actually allows to simplify the vector sections extraction again, removing the need to check for has_elseif.
I will let the nightly CI run against this branch to confirm whether it fixes the problem.
| of the conditional chain. | ||
| """ | ||
| if self.has_elseif: | ||
| return (self.else_body[0].body,) + self.else_body[0].else_bodies |
There was a problem hiding this comment.
Now I have understood - very neat indeed 😄
| @@ -141,7 +141,11 @@ def extract_vector_sections(cls, section, horizontal): | |||
| # as 'Conditional's rely on the fact that the first element of the 'else_body' | |||
| # (if 'has_elseif') is a Conditional itself | |||
| if separator.has_elseif and separator.else_body: | |||
There was a problem hiding this comment.
Since separator.else_bodies should, even in the has_elseif==False case, return the else_body correctly, we should be able to remove this check here and always use the new implementation, no?
mlange05
force-pushed
the
naml_fix_multicond_scc_vector
branch
from
ddc67d1 to
e4a4526
Compare
mlange05
force-pushed
the
naml_fix_multicond_scc_vector
branch
from
e4a4526 to
96cfb7d
Compare
|
Thanks for the early review; you were right, the logic could be drastically simplified. Unfortunately, last nights version was still buggy, but I've just rebase-pushed a clean version that should bring the tco399 ecphys regression back (confirmed locally). Please have another look, but should be ready now. |
Co-authored-by: Michael Staneker <[email protected]>
There was a problem hiding this comment.
The fixed fix seems to have resolved the problems in ifs-bundle. Many thanks for this!
I have accepted the comment suggestion from @MichaelSt98 as well.
reuterbal
added
the
ready for merge
The recent special-casing for else-if branches in the SCC vectorisation only works for single-level nesting, but skips any further nested else-ifs and the final else-branch. TO fix this, i've introduced a
else_bodiesproperty onConditionalnodes that returns the nested bodies for multi-conditionals. Using this, this we can then apply vector-section extraction for every else body in a deeply nested multi-conditional.