Port from Apps#644 to review in Psytrans also

[MetBenjaminWent]

I've done work in LFRic Apps #644 to generate Psyclone Transformation scripts for OpenMP on CPUs for across the domain.

Testing in this ticket and Apps#629 show that there is a beneficial performance change as a result of these scripts.

These are designed to be called by a generic global or override script which runs over all loops present.

It will span parallel sections where possible within list of rules.

Then will OpenMP the loops in line with rules provided, such as check for array of struct Nodes (types).

These have been checked in #644 with Pylint and flake8 in line with LFRic Python coding standards and pass.

[MetBenjaminWent]

Related to changes in https://code.metoffice.gov.uk/trac/lfric_apps/ticket/644.

Until there is a PsyTrans link accessible by Apps HoT, changes will also go onto trunk with Apps#644 to put performance benefits on trunk.

[MetBenjaminWent]

See issue #114

[MetBenjaminWent]

Based on our discussion this morning, I've done a slight refactor on the App Branch:

https://code.metoffice.gov.uk/trac/lfric_apps/changeset?reponame=&new=11203%40main%2Fbranches%2Fdev%2Fbenjaminwent%2Fr11135_v2_omp_transmute_glob&old=11193%40main%2Fbranches%2Fdev%2Fbenjaminwent%2Fr11135_v2_omp_transmute_glob

Will port relevant changes post your review.

I'm thinking making it obvious where the rules are might be helpful.

[oakleybrunt]

I'm stopping my review here for now. This needs refactoring and condensing. My main points I'd like you to look at are:

• Don't return boolean values from functions. Please throw exceptions which can be handled properly
• Stop storing boolean values which inform if statements within the same function. Just move the stuff that needs to be executed to that point!
• Stop using global variables

[oakleybrunt]

Please refrain from using global variables. If you need them within a function, just create the transformation object as and when you need it.

[MetBenjaminWent]

This is global as its a transformation that everything will use within that context.

I'd rather set them up once (like so), and use them as needed.

We could come up with a better storage solution sure that makes these as accessible (like in a class), but I think the way these are handled personally is fine.

Did you have a solution for where to store these? We could start a new script that is pre-designated calls in an object, but the result is the same really....

[oakleybrunt]

The class is in PSyclone, that's where you get it from. If you need to use one of these transformations, please make it within the function so it is easily readable - i.e. you can see exactly what was passed to OMPLoopTrans if you are just looking at the function.

Global variables are notoriously annoying when trying to read and understand code. "What is this variable?" "where did it come from?" are questions that I found myself asking when reviewing this change. Please remove them.

[MetBenjaminWent]

How about this as a solution?

It is called by:
trans = PropTrans()
trans.omp_transform_do()

'''
Getter wrappers for global transformations to be condensed and accessible
'''

from psyclone.transformations import (OMPLoopTrans, OMPParallelTrans)


class PropTrans:
    '''
    Class to store pre-configured transformations with getters
    '''
    def __init__(self):
        '''
        Initialise class with default properties
        '''
        
        # Setup transformations and their properties
        # OMP parallel do transformation
        self._omp_transform_par_do = OMPLoopTrans(
            omp_schedule="static",
            omp_directive="paralleldo")
        self._omp_parallel = OMPParallelTrans()
        self._omp_transform_do = OMPLoopTrans(
            omp_schedule="static",
            omp_directive="do")


    # Getters
    def omp_transform_par_do(self):
        '''
        Get pre configured omp_transform_par_do
        '''
        return self._omp_transform_par_do

    def omp_parallel(self):
        '''
        Get pre configured omp_parallel
        '''
        return self._omp_parallel

    def omp_transform_do(self):
        '''
        Get pre configured omp_transform_do
        '''
        return self._omp_transform_do

[oakleybrunt]

I think this could work as a solution but can you move this into a different PR please? For now, just call the transformations directly from PSyclone.

[oakleybrunt]

This function's docstring says it returns a schedule but it sometimes returns a bool. Please make it return a schedule or throw an exception

[MetBenjaminWent]

See the comment, I've updated this on the main branch on trac

[MetBenjaminWent]

I can update the doc string.

The intent though is to later check and return the presence of the object.

I don't want it throw an error.

I want it to return the schedule if there is one, and otherwise report there inst for checks that utilise.

[oakleybrunt]

Can you make it so the function returns None as default but return shed_list[0] if it gets to that point?

[MetBenjaminWent]

I can look at swapping this over if its possible.

I think I had some issues in the past converting a type none to an object in the past however

[MetBenjaminWent]

The None type in this instance seems to be working when converting from None to a Schedule, output is consistent post change, will add to commit with fix

[joewallwork]

On a quick scan, would it be possible to break this PR down slightly? It adds a lot of new functions but none of the tests to support them. Perhaps one module at a time, or (better still) a handful of functions at a time.

[joewallwork]

Any chance we could expand the name to get_last_child_schedule? Just because it's not immediately obvious that shed is meant to refer to schedule.

[joewallwork]

Remove debug prints