Renaming of classes and functions

[lczech]

Hey Sergey,

is it possible to rename classes and functions on the Python side? This could generally be a very useful token to have in the config file, for instance in the form of:

rename Old_name_as_it_would_be_created_by_binder NewNiceName

or the like. In particular, I am having some weirdly named class template instances:

namespace genesis {
namespace utils {
namespace math {

template class CompensatedSum<KahanSummation>;

} // namespace math
} // namespace utils
} // namespace genesis

(The class uses tag dispatch on the Summation class to select which algorithm to use for the compensated sum.)
However, this gives me a Python class that includes the namespace for the template type:

CompensatedSum_genesis_utils_math_KahanSummation_t

(and a kind of weird _t at the end as well), and I'd like to give this a nicer name such as KahanSum.

Thanks and so long
Lucas

[lyskov]

Currently there is no such functionality. As a workaround (and this is what I use) I recommend to consider shipping code alongside your shirred library (ie approach outlined in #290) that way you will get all kind of flexibility, like relocating particular symbols to different namespaces, wrapping them in helper functions and so on, - things that will be hard/impossible achieve in C++ level. I can point to code example in my project if you think that will be useful, - let me know.

[lczech]

Thanks for the suggestion, and see my reply in #290 as well. In short, I'd like to avoid to add downstream complexity by renaming things after the fact, i.e., after the import, and instead have the bindings be "clean" and resulting in what I actually want them to be. Otherwise, I'm afraid things will be harder to maintain. Happy to have a look at your example though and learning more about why your approach might actually be the better one - you have more experience with this!

As for the renaming: It seems that the Python function name is already a nice function, so at least at first glance, it seems easy enough to adapt that by augmenting it with a lookup for renaming? Or is there more to it? I could see if I can get this to work.

Cheers
Lucas

[lyskov]

My current view is that Binder job is to produce bindings for as close to C++ code as possible while the any additional Python-level-tweaking should be done at Python level. This is both due to technical issues (added code complexity for renaming and reordering) as well as "Write programs that do one thing and do it well" principle. But let me think about this, maybe there is another angle to it...

[lyskov]

re examples: it might be helpful to look at https://github.com/RosettaCommons/rosetta/tree/main/source/src/python/PyRosetta/src/pyrosetta/bindings and bindings for Pose (main protein container in Rosetta) in particular https://github.com/RosettaCommons/rosetta/blob/main/source/src/python/PyRosetta/src/pyrosetta/bindings/pose.py
My point is that unless project API structure is extremely lucky then it will be too-C++'ish and not enough Pythonic (and these two goals usually contradict each other) so some kind of bridge between these two representations is needed. For instance even the silly things like constructing vector like containers with specific types is trivial to define on Python level (see https://github.com/RosettaCommons/rosetta/blob/main/source/src/python/PyRosetta/src/pyrosetta/__init__.py#L248).

[lczech]

Thanks for the links, that's some good inspiration! And yes, I agree that some bridges and wrappers will be needed for any non-trivial project.

My current view is that Binder job is to produce bindings for as close to C++ code as possible

Yes, that is exactly what I am aiming for here as well :-)
In my example above, my C++ code just does this:

using KahanSum = CompensatedSum<KahanSummation>;

which is then the alias that I am using to create an instance of this class. However, as stated above, the LLVM processing in Binder resolves this to the underlying type, and also uses the namespaces in the template name, so that in Python I'd now have to refer to the same type as CompensatedSum_genesis_utils_math_KahanSummation_t instead of KahanSum, which is ugly.

To me, being able to rename this into what it is supposed to be is part of "the one thing" that I think Binder should be doing here. The automatic naming of template instances as currently implemented via LLVM type names is usually not resulting in Python code that is close to the C++ code.

So my suggestion is to add a config token rename (or rather +rename, if you want to keep the +/- prefixes), which creates a map used in python_function_name() to replace the auto-generated name.

What do you think? I could prepare a PR for that.

[lczech]

Hey @lyskov,

I have implemented the renaming directives now, see here. I think there are pretty good reasons to add this capability to Binder, see for instance my commends in the test case and documentation of the commit.

I will wait with opening a PR though, because we otherwise will probably get merge conflicts with my other PRs (in particular the stray clang-format one). It should work to base this commit here on PR #329 though without conflicts, as that already has the clang-format applied to it.

Let me know what you think, and so long
Lucas

[lyskov]

I see your point, - having direct way to rename items might be useful for simple projects. So PR adding this as a feature will be welcome!

A few thought on possible implementation:

-- current method does not allow user to change namespace of objects, might be a good idea to state that in documentation for clarity.

-- pattern here lczech@31eea25#diff-72f8d59ab463236c54ef8c3a2c8fb31c72efad5003751b3e342c868079de19baR597 is best done with using std::optional so we can avoid having unspoken contract that default value represent absence. However optional is not available in C++11 so it will be best to replace it instead with functions returning const reference to underlying map (see other config functions).

-- not sure if you want to add it so optional: might be possible to rename enums the same way.

[lczech]

So PR adding this as a feature will be welcome!

Sounds good, but shall we wait for the other PRs to be resolved, to avoid merge conflicts? I think that would just make both our lives easier :-)

current method does not allow user to change namespace of objects, might be a good idea to state that in documentation for clarity.

Sounds good.

pattern here lczech@31eea25#diff-72f8d59ab463236c54ef8c3a2c8fb31c72efad5003751b3e342c868079de19baR597 is best done with using std::optional so we can avoid having unspoken contract that default value represent absence. However optional is not available in C++11 so it will be best to replace it instead with functions returning const reference to underlying map (see other config functions).

Yeah, std::optional would be the way to go, but as you say, it's not available to us. I however wanted to avoid exposing internal containers, hence this. An empty string seems pretty much unmistakable to indicate a non-value.

not sure if you want to add it so optional: might be possible to rename enums the same way.

Sure, for completeness, that makes sense. I won't have time for that in the next couple of weeks though, but let's see :-)

[lyskov]

Sounds good, but shall we wait for the other PRs to be resolved, to avoid merge conflicts? I think that would just make both our lives easier :-)

-- Sure, could you please post (maybe here?) which order of PR merge will be the most convenient for you? I have limited so it is unlikely I will be able to review them all at once so let's concentrate on 1 maybe two issues/PR at the time.

Thanks,