Refactoring base Mapdl class.

[germa89]

This PR started to close #2082 by making sure I use wraps with all the wrapped functions, so I keep the doc string and such.

But it quickly growth to a full mapdl.py file refactoring. The _MapdlCore class was getting too long, so I split it in two main files:

• mapdl_core with the very base definition.
• mapdl_expanded where I expand Mapdl class with new functions or I expand the behaviour of already Mapdl existing.

I have also renamed the _MapdlCore class to MapdlBase. I keep this class definition in mapdl.py file. So it is clear MapdlBase should not be used.

New class dependency

Commands -> _MapdlCore -> _MapdlCommandExtended -> _MapdlExtended -> MapdlBase

1. Commands class that groups the MAPDL commands as they come from documentation parsing.
2. _MapdlCore added to the above the base methods, like checking type of instance, local, etc.
3. _MapdlCommandExtended: Extend MAPDL commands by wrapping/overwritting MAPDL commands from doc parsing.
4. _MapdlExtended: Add new functions to the above class.
5. MapdlBase: Just wrap the above with a better name. Allows for more subclassing from _MapdlExtended.

Close #2082

[ansys-reviewer-bot]

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

[codecov]

Codecov Report

Merging #2518 (f01e8f7) into main (5e70f2f) will increase coverage by 1.42%.
Report is 1 commits behind head on main.
The diff coverage is 83.98%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2518      +/-   ##
==========================================
+ Coverage   82.49%   83.91%   +1.42%     
==========================================
  Files          44       46       +2     
  Lines        9048     9071      +23     
==========================================
+ Hits         7464     7612     +148     
+ Misses       1584     1459     -125     

[germa89]

I do not care about coverage, since it is just a reorg.

[koubaa]

If this is true:

So it is clear MapdlBase should not be used.

Why do we use that name in the doc in so many places?

[koubaa]

I don't fully follow the class hierarchy. What is the difference between #1 and #3? Both levels have to do with doc parsing it appears

[germa89]

If this is true:

So it is clear MapdlBase should not be used.

Why do we use that name in the doc in so many places?

Fair point... because we need to refer to the three interfaces (grpc + console + corba?).... so we have used the parent class (from which the three of them are derived) to refer to it.

Probably not the best. We might need a bit of rethinking here.

[germa89]

I don't fully follow the class hierarchy. What is the difference between #1 and #3? Both levels have to do with doc parsing it appears

I cannot really relate the links to this issue. Can you explain better?

[koubaa]

I don't fully follow the class hierarchy. What is the difference between #1 and #3? Both levels have to do with doc parsing it appears

I cannot really relate the links to this issue. Can you explain better?

This is referring to the numbered list in the description of the PR. I forgot that github turned a hash followed by a number into a hyperlink =/

[germa89]

Oh.. Number 3 has "extended" parts ... like functions that wraps functions from parsing:

@wraps(Commands.cwd)
def cwd():
    # do some stufff
    super().cwd(*args, **kwargs)

I figured out it will be better to have been separated

[koubaa]

I generally tend to prefer composition over inheritance - in this case the backend could be a member of the Mapdl class, and then the user-facing class doesn't have to be a base class. And doesn't need a suffixed name like Core or Base

[germa89]

I generally tend to prefer composition over inheritance - in this case the backend could be a member of the Mapdl class, and then the user-facing class doesn't have to be a base class. And doesn't need a suffixed name like Core or Base

I dont know enough about composition. But I will investigate that option and come back with an informed opinion.

[germa89]

the user-facing class doesn't have to be a base class

The user-facing class will be MapdlGrpc or MapdlConsole. The user will never be able to use MapdlBase because it does not have backend. You could instantiate it, but it will raise a NotImplemented exception every time you run any command (because mapdl._run). This base class is just a container for all the methods that are common to MapdlGrpc and MapdlConsole.

[germa89]

Composition vs Inheritance

I generally tend to prefer composition over inheritance - in this case the backend could be a member of the Mapdl class, and then the user-facing class doesn't have to be a base class. And doesn't need a suffixed name like Core or Base

Having a read about composition vs inheritance, I end up in this stackoverflow question. So keeping in mind that pymapdl could have implemented the backend through composition as you mention, I believe that given the current relationship between MapdlBase and the child classes, where all the methods presented by the parent class are exposed by the child classes, I think inheritance is appropriate for this case.

Indeed, you could change it to composition, but I don't see much benefit.
Furthermore, you will have to come up with a way to overwrite/overload some methods in the parent class. In that case, I think you could go for, for not implement that method in the class, and just call the backend method. But I see that as a complication, because you will have to do that implementation in the other child classes (See example below).

I might not being using composition to its fullest, apologies about that. Or I might missed something. I'm happy to discuss this further though.

I must say also, that composition is used extensively in the MapdlBase class to implement the PostProcessing, Geometry, Mesh and Result classes (among others). I believe it is a nice way to expand a class, but I'm not sure it fits the current case.

Example

Given my base class:

class MapdlBase:
    def method1(self, *args, **kwargs):
        pass

   def set_backend(self, backend):
       self.backend = backend

Now imagine you want to change the method method1 for backend X.
You will have to do something like this in the base class:

class MapdlBase:
    def method1(self, *args, **kwargs):
        return self.backend.method1(*args,**kwargs)

   def set_backend(self, backend):
       self.backend = backend

This forces us to have in the other backends a method1 method, which in many cases will be all the same.

You could implement a switch in the MapdlBase class:

class MapdlBase:
    def method1(self, *args, **kwargs):
        if self.backend == "backend1":
            return self.backend.method1(*args,**kwargs)
        else:
             # general implementation
             pass

But I dont really like to have implementations backend-dependent in the general MapdlBase class.

[germa89]

Again...

I do not care about coverage, since it is just a reorg.