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

Add python bindings #6

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Add python bindings #6

wants to merge 11 commits into from

Conversation

ll-nick
Copy link
Collaborator

@ll-nick ll-nick commented Oct 18, 2024

As promised, here is a taste of what's to come :)

This contributes python bindings. Since this is a templated library, the python bindings have to be compiled at a later point in time, when the types are known. Therefore, we don't ship the bindings directly but rather add a convenience function that allows binding the types once the template arguments are known.

Now util_caching brings some extra challenges because it is possible to use it with very different kinds of keys. I solved it by adding two binding functions. One to bind a types you need for a number based key and an analogous one for time based keys.

When pybind is available and BUILD_TEST=TRUE, a python module is built that allows running unit tests (almost) equivalent to the currently implemented ones.

[EDIT] The following is outdated and has been fixed, see comment below.

There are two things missing right now. This line as well as this test.

The first one would require calling

python_api::bindTimeBasedCache<Time, double, std::chrono::seconds>(timeBasedSeconds);

Since these binds some of the same types as the existing statement, the resulting modules can't be loaded in the same python file.

Similarly, adding the SomePolicyWithoutParams would require a new module.

We could add those but in my opinion it would just add quite an overhead for a very small gain therefore I chose not to add it but we can of course discuss that.

@ll-nick
Copy link
Collaborator Author

ll-nick commented Oct 21, 2024

One idea I had one the weekend which might be a nice solution for allowing to add more comparison policies then the already implemented ones would be to add a template parameter pack and running

        .def(
            "cached",
            [](CacheT& self, const NumberT& key, const ApproximateNumberT& policy) {
                return self.template cached<ApproximateNumberT>(key, policy);
            },

for each passed policy. I'll set this PR back to draft and test that out first as it would solve the issue with the missing tests (I think).

@ll-nick ll-nick marked this pull request as draft October 21, 2024 06:02
@ll-nick ll-nick marked this pull request as ready for review October 21, 2024 09:05
@ll-nick
Copy link
Collaborator Author

ll-nick commented Oct 21, 2024

Ok, that actually worked out quite well.

I rearranged the bindings a little and added the missing unit tests. It is now possible to add a comparison policy as a template parameter to the bindCache function which will the overload the cached function for it. Check the comments in the code for a detailed explanation, I tried documenting all of it :)

@ll-nick
Copy link
Collaborator Author

ll-nick commented Oct 21, 2024

Rebased on current master.

@ll-nick
Copy link
Collaborator Author

ll-nick commented Nov 11, 2024

Rebased to main and adjusted to updated cmake structure.

@ll-nick
Copy link
Collaborator Author

ll-nick commented Nov 14, 2024

Rebased to main

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.

1 participant