search

precice / python-bindings

Python language bindings for preCICE
https://precice.org
GNU Lesser General Public License v3.0
22 stars 12 forks source link

Name of API function/class "Interface" can be misleading #92

Open ajaust opened 3 years ago

ajaust commented 3 years ago

The class Interface https://github.com/precice/python-bindings/blob/85245af97288c54b53d8ce26067408218aa99a0d/cyprecice/cyprecice.pyx#L34 can be misleading. It also deviates from the naming in the C++ API where the equivalent class is called SolverInterface.

This came up on Gitter where a user had troubles to create two interfaces for his coupling. They needed two meshes which they interpreted as two coupling interfaces and connected that to the Interface object in the Python bindings. I do not think that their way of thinking was completely wrong. I think that misunderstanding got even worse due to the usage of the term "interface" in the OpenFOAM adapter as this adapter was used by the user as well.

There might be additional (other) usage of the term "interface" in other adapters.

This problem does not appear all the time, but maybe we should discuss how one could avoid this misunderstanding.

BenjaminRodenberg commented 3 years ago

I totally agree. I at least cannot remember any real "reasoning" behind picking the term Interface over SolverInterface (which would be the reasonable default). Probably it was just always Interface and we never touched it.

BenjaminRodenberg commented 3 years ago

A remark on implementation: We can still provide Interface as a deprecated wrapper of SolverInterface then we can push when this change actually breaks user code until v3.0.0.1, if we want.

IshaanDesai commented 3 years ago

Some initial trials of changing the main API class name from Interface to SolverInterface shed light on the following issue:

The class SolverInterface already exists as the wrapper cppclass: https://github.com/precice/python-bindings/blob/85245af97288c54b53d8ce26067408218aa99a0d/cyprecice/SolverInterface.pxd#L6-L12 Hence using the class name again is not allowed. Renaming the SolverInterface class in the C++ wrapper conflicts with the original class name in the C++ API of preCICE. I assume this was the logic behind naming the Python class Interface and not SolverInterface. One solution could be wrap the C++ wrapper class into another class with a different name, like for example PRECICESolverInterface and then call that class pointer from the the renamed Python class SolverInterface. Would it be worthwhile to do this?

BenjaminRodenberg commented 3 years ago

In 86f249d @IshaanDesai and I implemented a workaround to avoid wrong usage. Replacing SolverInterface with Interface is possible, but at least to me currently not straightforward. I would suggest to postpone this issue for now, but anybody who has time to fix it, is, of course, invited to contribute :smirk:

BenjaminRodenberg commented 3 years ago

Let's quickly summarize the ideas we had and problems we saw or expected: