New hierarchy for Mapping classes

[Sworzzy]

Current situation

Currently in SymPDE there are two separate class hierarchies. The first one is internal to SymPDE and is used for symbolic mapping objects, which can be either undefined or analytic:

1. Undefined mappings are determined only by their name (a string) and by dim (an integer which represents the number of parametric dimensions);
2. Analytic mappings also carry explicit symbolic expressions for the coordinates (x, y, z), which are stored in the class attribute _expressions.

The base class of this hierarchy belongs to SymPy, hence the objects instantiated are also compatible with SymPy. The class inheritance diagram is:

IndexedBase [sympy]
     |
      — > BasicMapping
               |
                — > Mapping
                       |
                         — > IdentityMapping, AffineMapping, PolarMapping, ...

The second class hierarchy is for mappings whose coordinates (x, y, z) can be evaluated at a point in the logical domain, or at multiple points at once. Here the base class belongs to SymPDE and is the abstract class BasicCallableMapping, which defines an interface (i.e. the signature of all methods) for the concrete subclasses which can be instantiated.

In particular, SymPDE provides the concrete subclass CallableMapping which takes an analytic Mapping object as unique argument to the constructor, and uses the lambdify function of SymPy to generate standard Python functions from the symbolic expressions of the given object.

All mappings in Psydac are "callable", i.e. they can be evaluated at a point. In order to make them interchangeable with SymPDE callable mappings, all mapping classes in Psydac subclass the same abstract class BasicCallableMapping. Hence the class inheritance diagram for the second hierarchy is:

BasicCallableMapping (abstract: interface for "callable" mappings)
         |
         | — > CallableMapping
         |
           — > SplineMapping [psydac]
                     |
                      — > NurbsMapping [psydac]

In Psydac one deals with symbolic Mapping objects which "correspond to" spline or NURBS mappings. To represent this relationship, we attach a SplineMapping to an undefined Mapping object, thanks to the method Mapping.set_callable_mapping(F : BasicCallableMapping). Successive calls to get_callable_mapping will return the given object.

Of course this can be done with pure SymPDE mappings, as in

F_symb = PolarMapping(...)
F_eval = CallableMapping(F_symb)    # explicit, but...
F_symb.set_callable_mapping(F_eval) # easy to forget!

To avoid having to remember the last command, we can call the method Mapping.get_callable_mapping on the second line, because when called on an analytic mapping, it simply returns CallableMapping(self) and stores the result internally. Hence:

F_symb = PolarMapping(...)
F_eval = F_symb.get_callable_mapping()

The circular relationship between the classes Mapping and [Basic]CallableMapping is the main source of complexity which we would like to address in this PR.

Proposed improvement

This PR aims at cleaning the Psydac-SymPDE mapping interface. There is now only one mapping hierarchy

                                  SplineMapping    -->   NURBS
                               /    (Psydac)            (Psydac)
                              / 
IndexBase  -->  BaseMapping - 
 (SymPy)         (SymPDE)     \
                               \
                                BaseAnalyticMapping   -->  PolarMapping, AffineMapping, IdentityMapping ... 
                                     (SymPDE)                (SymPDE)      (SymPDE)        (SymPDE)

The main features are :

• getting rid of callable mappings. You can now deal with analytic or spline mappings and do point / domain / array / meshgrid - wise evaluations.

• All the symbolic calculus features in the previous Mapping class are preserved in BaseMapping. It's essentially the same constructor.

• BaseAnalyticMapping is just BaseMapping constructor added with the lambdify_sympde functions, to make evaluation methods.

In this branch, instantiating spline mapping and analytic mapping is like previously.

Improvements may be done : the meshgrid evaluation method doesn't support the sparse meshgrid format.

[yguclu]

Given the CI failures, it appears that not every name and import path has been updated in the unit tests.

[Sworzzy]

There is a fail in topology/tests/test_logical_expr.py more specifically in test_collela_mapping_2d_1 (l.44), because the expressions of the CollelaMapping2D don't match the ones expected in the test. Maybe we should change the expressions expected in the test, or there's something I'm missing.

[yguclu]

There is a fail in topology/tests/test_logical_expr.py more specifically in test_collela_mapping_2d_1 (l.44), because the expressions of the CollelaMapping2D don't match the ones expected in the test. Maybe we should change the expressions expected in the test, or there's something I'm missing.

Yes, we should update the test.

[yguclu]

The original class BasicCallableMapping is abstract, in the sense that:

1. No objects of this class can be instantiated (but objects of concrete subclasses can)
2. Subclasses must implement every abstractmethod in order to be concrete

I feel that these two properties are very desirable. Instead, the new class BaseMapping does not define an abstract interface, hence there is no guarantee that its subclasses BaseAnalyticMapping (in SymPDE) and SplineMapping (in Psydac) can be used interchangeably. In practice you still give these subclasses methods with the same name and arguments, I suppose?

[Sworzzy]

You are absolutely right. Having a parent abstract base class is the best scenario, and we did try to have it in our final hierarchy. Here's why we didn't keep it :

As I have written in my Psydac PR in my first comment https://github.com/pyccel/psydac/pull/429 :

Psydac uses "domain undefined mapping", like in the Domain.from_file / Geometry.read methods.

"domain undefined mapping" are Domain objects that have a mapping attribute, and the mapping ONLY has a name and dim attribute.

One may think that the class that is to be used to instantiate those "undefined mapping" is the parent class (as it was suggested in the old hierarchy). This was impossible to do since the parent was an abstract.

Thus we chose a less elegant way, where we would have a instantiatable parent class, that could support the "undefined mappings".
This class defines what would be the "abstract methods" with just using pass, and the methods in each subclass have the same name.

If you want an abstract base class, here are suggestions :

-> adapt the code in Psydac like the methods I mentioned earlier so that we can only deal with defined mappings (callable)

-> create an AbstractMapping that is parent to BaseMapping. You could use this AbstractMapping in the pull_push files (replaces the old BasicCallableMapping).

I think the second option is the best, because I believe that undefined mapping have a very powerful yet subtle role in the symbolic calculus process and so you shouldn't get rid of it. You would still be able to support undefined mappings with the BaseMapping class.

[campospinto]

After discussing this issue, another option is to have a concrete class for "undefined mappings" and an abstract class below for "defined mappings", which would then be subclassed by spline or analytical mappings.

So, something like:

IndexBase [sympy]
  |
   — > UndefinedMapping [sympde]
         |
          — > Mapping [sympde] (abstract: interface for "callable" ie evaluable mappings)
                | \
                |   — > AnalyticMapping [sympde]
                |         |
                |          – > PolarMapping, AffineMapping, IdentityMapping ... 
                 \
                   —– > SplineMapping [psydac] 
                          |
                           — > NURBS [psydac]

[campospinto]

Given the CI failures, it appears that not every name and import path has been updated in the unit tests.

@Sworzzy did you manage to fix the unit tests ?

There is a fail in topology/tests/test_logical_expr.py more specifically in test_collela_mapping_2d_1 (l.44), because the expressions of the CollelaMapping2D don't match the ones expected in the test. Maybe we should change the expressions expected in the test, or there's something I'm missing.

Yes, we should update the test.

This problem was actually caused by the fact that the SymPDE version of CollelaMapping2D had been changed to another variant (used in some unrelated Psydac test) which was not compatible. We (Patrick and I) put back the SymPDE version.