Fix edge case for vector section mapping

[MichaelSt98]

conflicting with (rebuild of) Conditional.

Fixes #380

[github-actions]

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/382/index.html

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.49%. Comparing base (cc179b3) to head (e5d00c5).
Report is 13 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #382   +/-   ##
=======================================
  Coverage   95.48%   95.49%           
=======================================
  Files         185      185           
  Lines       38646    38664   +18     
=======================================
+ Hits        36902    36921   +19     
+ Misses       1744     1743    -1     

Flag	Coverage Δ
lint_rules	96.39% <ø> (ø)
loki	95.47% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[reuterbal]

Nice find! The fix is correct but I think we need to distinguish between the case where this is an actual else-if and where this is just an if nested into an else.

[reuterbal]

Note that this is only required if the Conditional actually has has_elseif == True.
The following two version will create the same IR, with the subtle difference that the outer conditional has has_elseif == True

IF (A > 0) THEN
  A = A + 1
ELSE
  IF (A < 0) THEN
    A = A - 1
  ENDIF
ENDIF

vs.

IF (A > 0) THEN
  A = A + 1
ELSEIF (A < 0) THEN
  A = A - 1
ENDIF

For readability, I would also prefer to use proper if/else blocks rather than putting everything in a single line with line breaks. I would suggest rewriting this as

Suggested change

	subsec_else = cls.extract_vector_sections(separator.else_body[0].body, horizontal)\
	if separator.else_body and isinstance(separator.else_body[0], ir.Conditional)\
	else cls.extract_vector_sections(separator.else_body, horizontal)
	if separator.has_elseif and separator.else_body:
	subsec_else = cls.extract_vector_sections(separator.else_body[0].body, horizontal)
	else:
	subsec_else = cls.extract_vector_sections(separator.else_body, horizontal)

[MichaelSt98]

Yes! There is just a problem with OMNI as the parsing of Conditionals is a bit different to FP and OFP and has_elseif is always False for OMNI. Thus I could do what you suggest and disable OMNI or "fix" Conditional parsing/construction for OMNI.
What do you think?

[mlange05]

I think the OMNI-failures are actually due to the fix being correct, and OMNI treatingelseif-constructs wrong. However, I don't think there is much you can do to "fix OMNI" in this case, as it automatically expands every ELSEIF into a nested ELSE => IF code structure in its frontend, even before we get the AST. So, unless we reconstruct the correct ELSEIF by inspecting the source string, there is no catching this.

Usually, I'd say this leaves only special-casing the test, but since this quickly becomes cumbersome, I think we can just safely this test (with an explicit mention of OMNI's elseif-expansion).

[reuterbal]

Agreed! The transformation's behaviour should be governed by the IR it receives, and not try to restore what OMNI has expanded previously. Importantly, OMNI does not change the meaning, simply the appearance. So, disabling the test for Omni (or hacking the IR by setting the flag before calling the transformation if you think that's better) seems sensible to me.

[reuterbal]

Thanks, looks great now! Will also do a rebase for sanity.

[mlange05]

Looks good. GTG.