python-bindings icon indicating copy to clipboard operation
python-bindings copied to clipboard

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

Open ajaust opened this issue 3 years ago • 5 comments

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.

ajaust avatar Mar 05 '21 11:03 ajaust

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 avatar Mar 05 '21 13:03 BenjaminRodenberg

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.

BenjaminRodenberg avatar Mar 05 '21 18:03 BenjaminRodenberg

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?

IshaanDesai avatar Mar 08 '21 13:03 IshaanDesai

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 avatar Mar 09 '21 11:03 BenjaminRodenberg

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

  • Just writing from cyprecice import Interface as SolverInterface works as a wrapper, but then the name in the documentation stays Interface
  • Renaming Interface to SolverInterface here causes a name clash.
  • We could also rename cyprecice/SolverInterface.pxd to cyprecice/cySolverInterface.pxd, but this might again lead to problems with the included SolverInterface.hpp file that we cannot change. We did not try it, though.

BenjaminRodenberg avatar Mar 09 '21 12:03 BenjaminRodenberg