lisitsyn / tapkee

A flexible and efficient С++ template library for dimension reduction
http://tapkee.lisitsyn.me
BSD 3-Clause "New" or "Revised" License
231 stars 58 forks source link

Nanobind #99

Closed iglesias closed 2 months ago

iglesias commented 4 months ago

I thought next to be adding to this PR (1) a workflow to have the Python interface in the CI and (2) more of the parameters in nanobind_extension.cpp. I should also try using other of the DR methods from Python and consider if it could be nice (and easy) to have different combinations covered in the CI too.

And well also the bunch of TODO's I left ':-D

So, hmm, there could be a lot. I think I will continue pushing step by step and keep an eye how it can start getting integrated.

iglesias commented 3 months ago

Running in the CI looking good too! https://github.com/iglesias/tapkee/actions/runs/9153626706/job/25162869366

I will update the workflow since it would be nice if the new job appears in the pull request too.

iglesias commented 3 months ago

We saw that it worked with nanobind and now know what to expect from it. It can be picked up at any moment when there's interest in having it with yesterday's new parse_* methods and templates.

iglesias commented 3 months ago

I quickly merged the upstream changes in examples/go.py. Next, I recall from two weeks ago there had also been changes in the templates, so I expect the nanobind build to fail.

iglesias commented 3 months ago

I started updating the extension and got to this one:

src/python/nanobind_extension.cpp: In function ‘void nanobind_init_tapkee(nanobind::module_&)’:
src/python/nanobind_extension.cpp:38:10: error: no matching function for call to ‘nanobind::module_::def(const char [15], <unresolved overloaded function type>)’
   38 |     m.def("parse_multiple", &parse_multiple<DimensionReductionMethod>);
      |     ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from venv-tapkee-nanobind/lib/python3.12/site-packages/nanobind/include/nanobind/nanobind.h:51:venv-tapkee-nanobind/lib/python3.12/site-packages/nanobind/include/nanobind/nb_func.h:255:10: note: candidate: ‘template<class Func, class ... Extra> nanobind::module_& nanobind::module_::def(const char*, Func&&, const Extra& ...)’
  255 | module_ &module_::def(const char *name_, Func &&f, const Extra &...extra) {
      |          ^~~~~~~
venv-tapkee-nanobind/lib/python3.12/site-packages/nanobind/include/nanobind/nb_func.h:255:10: note:   template argument deduction/substitution failed:
src/python/nanobind_extension.cpp:38:10: note:   couldn’t deduce template parameter ‘Func’
   38 |     m.def("parse_multiple", &parse_multiple<DimensionReductionMethod>)
iglesias commented 3 months ago

I started updating the extension and got to this one:

src/python/nanobind_extension.cpp: In function ‘void nanobind_init_tapkee(nanobind::module_&)’:
src/python/nanobind_extension.cpp:38:10: error: no matching function for call to ‘nanobind::module_::def(const char [15], <unresolved overloaded function type>)’
   38 |     m.def("parse_multiple", &parse_multiple<DimensionReductionMethod>);
      |     ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from venv-tapkee-nanobind/lib/python3.12/site-packages/nanobind/include/nanobind/nanobind.h:51:venv-tapkee-nanobind/lib/python3.12/site-packages/nanobind/include/nanobind/nb_func.h:255:10: note: candidate: ‘template<class Func, class ... Extra> nanobind::module_& nanobind::module_::def(const char*, Func&&, const Extra& ...)’
  255 | module_ &module_::def(const char *name_, Func &&f, const Extra &...extra) {
      |          ^~~~~~~
venv-tapkee-nanobind/lib/python3.12/site-packages/nanobind/include/nanobind/nb_func.h:255:10: note:   template argument deduction/substitution failed:
src/python/nanobind_extension.cpp:38:10: note:   couldn’t deduce template parameter ‘Func’
   38 |     m.def("parse_multiple", &parse_multiple<DimensionReductionMethod>)

I am still working on other build errors after this one. For this one the DimensionReductionMethod is not the right template, it'd be a map.

lisitsyn commented 3 months ago

I started updating the extension and got to this one:

src/python/nanobind_extension.cpp: In function ‘void nanobind_init_tapkee(nanobind::module_&)’:
src/python/nanobind_extension.cpp:38:10: error: no matching function for call to ‘nanobind::module_::def(const char [15], <unresolved overloaded function type>)’
   38 |     m.def("parse_multiple", &parse_multiple<DimensionReductionMethod>);
      |     ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from venv-tapkee-nanobind/lib/python3.12/site-packages/nanobind/include/nanobind/nanobind.h:51:venv-tapkee-nanobind/lib/python3.12/site-packages/nanobind/include/nanobind/nb_func.h:255:10: note: candidate: ‘template<class Func, class ... Extra> nanobind::module_& nanobind::module_::def(const char*, Func&&, const Extra& ...)’
  255 | module_ &module_::def(const char *name_, Func &&f, const Extra &...extra) {
      |          ^~~~~~~
venv-tapkee-nanobind/lib/python3.12/site-packages/nanobind/include/nanobind/nb_func.h:255:10: note:   template argument deduction/substitution failed:
src/python/nanobind_extension.cpp:38:10: note:   couldn’t deduce template parameter ‘Func’
   38 |     m.def("parse_multiple", &parse_multiple<DimensionReductionMethod>)

I am still working on other build errors after this one. For this one the DimensionReductionMethod is not the right template, it'd be a map.

I'd suggest to introduce some currying so that there is an additional parse_method that just fixes the first argument of parse_multiple. This way you won't have to call a templated function. What do you think?

iglesias commented 3 months ago

I'd suggest to introduce some currying so that there is an additional parse_method that just fixes the first argument of parse_multiple. This way you won't have to call a templated function. What do you think?

Yeah! I think that's a good idea. It'd also simplify the extension in avoiding to expose the maps.

iglesias commented 3 months ago

The extension is back: https://github.com/iglesias/tapkee/actions/runs/9535436521/job/26281156242

There was a funny interaction with the renaming from withParameters to with since, I guess, it's a reserved word in Python.

For next, it appears the branch cannot be rebased due to conflicts. I haven't looked into it yet.

iglesias commented 2 months ago

Ah, all right, I think I understand about the rebase now. It can be squashed at least, I think that's suitable for this one. I think it can be considered in a state to be ready for merge, then, it enables using the chain interface nicely from Python as well as directly inputting np.arrays to embed 🎉

lisitsyn commented 2 months ago

Do you think we squash and merge or would you force-squash it?

iglesias commented 2 months ago

I can do it here right away.