PSyclone icon indicating copy to clipboard operation
PSyclone copied to clipboard

Adding unique classes for each kernel type in the LFRic domain

Open oakleybrunt opened this issue 1 year ago • 3 comments

In the LFRic domain, PSyclone has only one user-defined kernel class, LFRicKern. This one class is responsible for managing generic, domain, intergrid, and eventually dof kernels. This has made the LFRicKern class quite bloated with conditionals, and the addition of the dof kernel checks will only add to this.

@TeranIvy proposed that we should split the LFRicKern class into unique classes for each aforementioned kernel types. We would like to set up this issue to discuss how this might be achieved and weed out any problems with this approach.

oakleybrunt avatar Jun 19 '24 10:06 oakleybrunt

If LFRicKern and LFRicKernMetadata were split, there would have to be some way of determining which kernel type to initialise for each invoke. This could maybe live in LFRicAlg::kernel_from_metadata() with similar conditional checks in KernelTypeFactory to initialise the correct metadata class.

oakleybrunt avatar Jun 19 '24 15:06 oakleybrunt

Alternatively, LFRicKern and LFRicKernMetadata could become children with multiple inheritance from the distinct kernels. Their role would be to distinguish which LFRic kernel type to initialise and return an instance of the correct kernel.

In any case there is a common parent LFRic kernel for all unique kernels in the LFRic domain. This is needed to set all the attributes currently held by LFRicKern, since without them there would have to be many added checks for which kernel has been initalised.

oakleybrunt avatar Jun 20 '24 08:06 oakleybrunt

This all sounds sensible and good to me. However, I would avoid multiple inheritance (i.e. where a class inherits from more than one parent class) if possible as that can make life difficult (we've found this to our cost already). We would perhaps want a LFRicBaseKern and a LFRicBaseKernMetadata class for all the common functionality. We would then have e.g. LFRicCMAKern, LFRicDoFKern, LFRicInterGridKern etc. which would all sub-class LFRicBaseKern. We'd do something similar for the metadata classes of course.

arporter avatar Jul 01 '24 08:07 arporter

This all sounds sensible and good to me. However, I would avoid multiple inheritance (i.e. where a class inherits from more than one parent class) if possible as that can make life difficult (we've found this to our cost already).

What about using mixins? They seem like an obvious solution for the problems of similar or identical methods between some - but not all - kernel classes.

oakleybrunt avatar Jul 08 '24 08:07 oakleybrunt

What about using mixins? They seem like an obvious solution for the problems of similar or identical methods between some - but not all - kernel classes.

Yes, a Mixin is a good solution.

arporter avatar Jul 08 '24 08:07 arporter

As discussed with @arporter, there is already work done that splits the LFRic kernel metadata into component parts. The current LFRicKern does not use this metadata class, instead it uses the LFRicKernMetadata class. Until the new metadata class is in use, this has been postponed.

oakleybrunt avatar Aug 09 '24 12:08 oakleybrunt