Interface refactor #343
Interface refactor #343
Conversation
| @@ -33,8 +33,90 @@ set(CMAKE_CXX_STANDARD 11) | |||
|
|
|||
| include_directories( | |||
| include/ | |||
| src/kbmod/include/ | |||
There was a problem hiding this comment.
Hmm, a bit strange to have two include directories for the same code. Python doesn't care about the top-level include dir, so do you want to merge it all all the way down? Optionally we could adopt the more classic C++ way of project organizing where we split up our headers into include and implementation into src. That's how afw did it but I'm not sure what the implicit tradeoffs of doing it like that are. It definitely looks a lot more complicated.
With that also, maybe you could explain to me, here or in a meeting, what are the traditional structures used in C++ projects to manage dependencies? The nvidia headers of error declarations - ok whatevs. But including the whole fitsio.h and hoping it exists out there in the universe somewhere has been the weirdest thing to me.
src/kbmod/image_base/bindings.cpp
Outdated
|
|
||
| using std::to_string; | ||
|
|
||
| PYBIND11_MODULE(search, m) { |
There was a problem hiding this comment.
It's important to remember that we can only have one PYBIND11_MODULE declaration for the whole codebase, so stuffing it into image_base is probably not the way. Let me come back here in a minute with a proposal how to manage this file.
There was a problem hiding this comment.
Interesting. I didn't realize that was a restriction. We should chat about how we can break things up then.
Proposal on how to split Pybind bindings into multiple files for readability. Implemented on the example of a PSF class. The code currently doesn't work because we can not have multiple entry point calls to the macro PYBIND11_MODULE in a single codebase. Or at least I don't think we can and nothing I tried worked. We certainly can not extend a once-declared module by repeatedly calling PYBIND11_MODULE, but seemingly we can not define a new one either.
|
I just pushed a proposal example on the PSF class of how to split the implementation up into multiple functions. I believe the C++ code can be made to be self-standing in this way as well (i.e. it can be compiled independently of the presence of Pybind with some modifications to the CMake script). I have no idea if that's useful at all. I saw that you wanted to split up the modules via multiple >>> import kbmod.search
>>> dir(kbmod.search)
['STAMP_MEAN', 'STAMP_MEDIAN', 'STAMP_SUM', 'StampType', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__spec__', 'calculate_likelihood_psi_phi', 'sigmag_filtered_indices', 'stack_search', 'stamp_parameters', 'trajectory']
>>> import kbmod.image_base
>>> dir(kbmod.image_base)
['__doc__', '__file__', '__loader__', '__name__', '__package__', '__path__', '__spec__']
>>> There's just nothing in the module at all. The proposed workaround is to have a bindings code, including the pythonic docstrings, for each of the classes we interface to Python instead. One thing I don't like is that the factories, because the implementation is in the "header" gets duplicated everywhere so I've declared all as "static" I don't know if that's the best approach, or what the tradeoff is, but how the code gets unraveled by the linker is somewhat of a mystery to me. EDIT: Oh, and I put the bindings into their own namespace, but I'm not particularly strongly married or divorced to the idea. |
|
Closing as we decided not to go down this path at the moment and Dino already implemented the binding refactor in #346 |
Draft PR for discussions on proposed refactoring