(Towards #2156) Changes to routine_copy and fortran backend for elemental and impure support

[LonelyCat124]

A few small changes required to make the PSyIR layer (mostly the backends) not lose information on pure and elemental subroutines.

This might solve some of the NEMO issues reported, but only if the subroutines are contained within modules.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.86%. Comparing base (453bfa6) to head (2b8cc6d).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2566   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         354      354           
  Lines       48330    48411   +81     
=======================================
+ Hits        48267    48348   +81     
  Misses         63       63           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[LonelyCat124]

I've had to change something in fparser2.py now as well - the default behaviour is now that the purity of a subroutine is None (i.e. unknown) unless it is specified. This is required as elemental implies pure unless specified, however an input of elemental without pure will result in impure elemental with the old implementation, and I believe unknown => pure.

We could also adapt this to have elemental set is_pure=True unless is_pure is already False, but I will leave that to the reviewer.

@arporter this is ready for review now - may be helpful to have before Nemo party but I'm not sure if its sufficient to fix their issues.

[LonelyCat124]

Also I realise that this doesn't technically fix the stuff for the issue 2156 but just andy's comments within.

[LonelyCat124]

@arporter @sergisiso Should we also ask to test this branch with NEMO before the NEMO party?

[LonelyCat124]

@arporter changes done so should be ready for another look once the runner on glados is up and running again.

[sergisiso]

Thanks @LonelyCat124 to implement this, looking at NEMO upstream it seems that they all are elemental functions, I think it will work with your implementation but it would be good to add some tests with functions to make sure it all goes right(frontend-attribute-backend). The signatures in nemo are: ELEMENTAL FUNCTION SIGN_SCALAR( val1, val2 ) and there is one: impure ELEMENTAL FUNCTION pres_temp_sclr( val)

[LonelyCat124]

Thanks @LonelyCat124 to implement this, looking at NEMO upstream it seems that they all are elemental functions, I think it will work with your implementation but it would be good to add some tests with functions to make sure it all goes right(frontend-attribute-backend). The signatures in nemo are: ELEMENTAL FUNCTION SIGN_SCALAR( val1, val2 ) and there is one: impure ELEMENTAL FUNCTION pres_temp_sclr( val)

I'm guessing there's some return type on these functions too? It works if I use real elemental function but I get a codeblock from something else if its just elemental function x

Edit: Never mind, I made it work without the return type declared on the function now.

Tests added - ready for another look

[sergisiso]

In nemo functions the type is in the declarations (with the same name as the function), not the function line:

   ELEMENTAL FUNCTION SIGN_SCALAR( pa, pb )
      !!-----------------------------------------------------------------------
      !!                  ***  FUNCTION SIGN_SCALAR  ***
      !!
      !! ** Purpose : overwrite f95 behaviour of intrinsinc sign function
      !!-----------------------------------------------------------------------
      REAL(wp), INTENT(in) :: pa,pb          ! input
      REAL(wp)             :: SIGN_SCALAR    ! result
      !!-----------------------------------------------------------------------
      IF ( pb >= 0._wp ) THEN   ;   SIGN_SCALAR =  ABS(pa)
      ELSE                      ;   SIGN_SCALAR = -ABS(pa)
      ENDIF
   END FUNCTION SIGN_SCALAR

[LonelyCat124]

Yeah - I put that in the tests and it works. PSyclone does just create a codeblock if I have real elemental function but I don't know why that is, I assumed it was a generic PSyclone thing and not related to this PR.

[LonelyCat124]

Also - should I do something on xfailing tests that makes the test fail if the xfail condition isn't met? I feel like those tests should either xfail or fail - if they succeed we should have a proper test for them (I just realised while playing around with #2201 that if I make the xfail test work it doesn't report anything wrong) - adding an else in the test fails coverage.

[arporter]

Also - should I do something on xfailing tests that makes the test fail if the xfail condition isn't met? I feel like those tests should either xfail or fail - if they succeed we should have a proper test for them (I just realised while playing around with #2201 that if I make the xfail test work it doesn't report anything wrong) - adding an else in the test fails coverage.

There is a pytest flag to have x-passes reported as failures but the trouble is we have some tests that we know pass locally but fail on GHA for reasons we've never quite managed to get to grips with. You can protect the else with a pragma to say to ignore coverage for it.

EDIT: we could probably have a pytest config file that's only used on GHA but that's starting to get quite complicated.

[arporter]

Yeah - I put that in the tests and it works. PSyclone does just create a codeblock if I have real elemental function but I don't know why that is, I assumed it was a generic PSyclone thing and not related to this PR.

Would you mind trying to fix this? It should work as we support the two things independently (i.e. real function x and elemental function x).

[LonelyCat124]

Also - should I do something on xfailing tests that makes the test fail if the xfail condition isn't met? I feel like those tests should either xfail or fail - if they succeed we should have a proper test for them (I just realised while playing around with #2201 that if I make the xfail test work it doesn't report anything wrong) - adding an else in the test fails coverage.

There is a pytest flag to have x-passes reported as failures but the trouble is we have some tests that we know pass locally but fail on GHA for reasons we've never quite managed to get to grips with. You can protect the else with a pragma to say to ignore coverage for it.

EDIT: we could probably have a pytest config file that's only used on GHA but that's starting to get quite complicated.

I found a better way to do this - I assert the fail condition then call pytest.xfail after that.

Would you mind trying to fix this? It should work as we support the two things independently (i.e. real function x and elemental function x).

I'm running some tests for 2201 at the moment, once those complete I'll try to fix it.

[LonelyCat124]

@arporter I'm not sure this was actually broken, but I think my syntax was the issue.

real impure elemental function sub(...) gives me a Routine object as expected. I'll add this into the other function tests as well.

[arporter]

Just a note to record that the NEMO and compilation integration tests were fine for this PR. (LFRic tests are currently not working.)

[LonelyCat124]

Ready for another look now, let me know if you think the comment is sufficient.