Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend ProfileDictionary in Python #59

Conversation

dave992
Copy link

@dave992 dave992 commented Oct 14, 2023

In #51 I saw a comment in the code of adding a add_profile function for the ProfileDictionary class.

I have created a small example that could do that using the available wrapper functions and extending the ProfileDictionary class in Python. I assumed for this example it is possible to extend a .so library with Python code.

I am curious if this approach of extending some of the wrapped classes in Python to mimic the C++ functionality is something you would consider for this repository or if you would rather update the SWIG configuration to achieve the same/similar.

The example as is would likely cause some name clash, but I assume that the C++ ProfileDictionary could be placed in a separate namespace or be renamed in the binding to do effectively the same thing.

@johnwason
Copy link
Contributor

The design is intentional to decouple the different modules. The current wrappers follow the C++ library exactly, and do not attempt to make things more pythonic. In particular the memory management follows the underlying rules of the C++ library and will segfault if they are not followed correctly. We are thinking about making a higher level adapter that provides a more pythonic interface, but that would be a separate module from these wrappers.

@johnwason johnwason closed this Oct 14, 2023
@dave992
Copy link
Author

dave992 commented Oct 14, 2023

The design is intentional to decouple the different modules.

Okay, I assumed the different ProfileDictionary_addProfile_* were only there due to some difficulties exposing the addProfiles function directly instead of it being intentional.

I am not really sure I understand how this decouples the modules. The compiler still needs to pull in the definition of the profiles to create those functions, so on the C++ side, the modules are still coupled. If Swig exposed the addProfiles member functions directly, this would result in the same situation as far as I understand. Looking at the bindings it created, all Python code directly calls the compiled library and passes on all arguments as is. Only inside the compiled library, the corresponding overload is chosen if available.

The current wrappers follow the C++ library exactly and do not attempt to make things more pythonic.

I did not attempt to make the wrapper more Pythonic, just have a user have the exact same interface and member functions as in C++. For the ProfileDictionary class specifically, there exists a ProfileDictionary::addProfiles member function. This does not exist for the Python equivalent.

@johnwason
Copy link
Contributor

addProfiles is a template function. Templates in C++ are not generics, and the implementation is only generated when it is used. For C++, this is transparent and handled automatically by the compiler, but for SWIG it needs to be explicitly instantiated. Take a look at the Trajopt unit test. Note this import:

from tesseract_robotics.tesseract_motion_planners_trajopt import TrajOptDefaultPlanProfile, TrajOptDefaultCompositeProfile, \
    TrajOptProblemGeneratorFn, TrajOptMotionPlanner, ProfileDictionary_addProfile_TrajOptPlanProfile, \
    ProfileDictionary_addProfile_TrajOptCompositeProfile

Because the TrajoptDefaultPlanProfile exists in the tesseract_motion_planners_trajopt module, the corresponding ProfileDictionary_addProfile_TrajOptPlanProfile also exists in that module. This implementation detail of generating the addProfile implementation for TrajoptDefaultPlanProfile would be handled transparently by the C++ compiler, but needs to be handled manually in Python with SWIG.

@dave992
Copy link
Author

dave992 commented Oct 14, 2023

addProfiles is a template function. Templates in C++ are not generics, and the implementation is only generated when it is used. For C++, this is transparent and handled automatically by the compiler, but for SWIG it needs to be explicitly instantiated

Yes, that's what I tried to describe.

However, by creating the ProfileDictionary_addProfile_TrajOptPlanProfile function you also explicitly instantiate a function for the TrajoptDefaultPlanProfile type. I am not sure how this is different to explicitly defining a templated variant for addProfiles.

Effectively a function is moved from one module to another, disconnecting it from its original "parent" class. The connection between tesseract_command_language and tesseract_motion_planners_trajopt still exists inside the library to allow that function to exist, just as would've be the case when addProfile is exposed directly.

I am not sure I understand how the import is relevant. The ProfileDictionary_addProfile_ is never used on its own as it needs an instance of ProfileDictionary. If addProfile was a member function of ProfileDictionary in Python, it would not need any import as you can just use the object and call the function on it. I do not need to import to use an instance of it. Only if you need to create the object you need to import, but that's valid now as well.

Thanks for the answers in any case, it did make some elements more clear to me.

@dave992 dave992 deleted the feature/profile_dictionary_add_profiles branch October 15, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants