DM-32008: Wrap test_spoke in C++/pybind11
#152
Conversation
morriscb
changed the title
test_spoke in C++/pybind1test_spoke in C++/pybind11
|
|
||
| namespace { | ||
|
|
||
| int testSpoke(double cos_theta_src, double sin_theta_src, py::array_t<double> ref_ctr, int ref_ctr_id, |
There was a problem hiding this comment.
I think this function should be implemented in a file in src/, with a header in include/ (which then gets the docstrings that this definitely needs!); the only thing in this pybind file should be the PYBIND11_MODULE below.
| int testSpoke(double cos_theta_src, double sin_theta_src, py::array_t<double> ref_ctr, int ref_ctr_id, | ||
| py::array_t<double> proj_ref_ctr_delta, double proj_ref_ctr_dist_sq, | ||
| py::array_t<unsigned int> ref_dist_idx_array, py::array_t<unsigned int> ref_id_array, | ||
| py::array_t<double> reference_array, double src_sin_tol) { |
There was a problem hiding this comment.
These py::array_t should be ndarray::Array<type, 1> when you move this to src/. Then I think you won't need the unchecked iterators below, you can just do ref_ctr[0], etc.
morriscb
force-pushed
the
tickets/DM-32008
branch
2 times, most recently
from
c91751b
to
7f637bc
Compare
|
Finished things up mostly except for one compiler warning. |
|
Please squash your commits. |
| * no match is found. | ||
| */ | ||
| int check_spoke(double cos_theta_src, double sin_theta_src, ndarray::Array<double, 1, 1> const& ref_ctr, | ||
| uint16_t ref_ctr_id, ndarray::Array<double, 1, 1> const& proj_ref_ctr_delta, |
There was a problem hiding this comment.
Is ref_ctr_id really a uint16? That means the refcat can only have 65k entries. I'd expect this, ref_id_array, and ref_dist_idx_array to all be std::size_t, since they are array indexes.
There was a problem hiding this comment.
It is. I'll change the other two types and see if it compiles.
There was a problem hiding this comment.
Did these not work as size_t? The uint16 makes me uncomfortable.
|
|
||
| PYBIND11_MODULE(pessimisticPatternMatcherUtils, mod) { | ||
| mod.def("check_spoke", &check_spoke, | ||
| "Test the opening angle between the spokes of our source pattern against the reference."); |
There was a problem hiding this comment.
Huh, I wasn't aware that you could add a python docstring like this. We don't mention it in the dev guide, but we probably should!
| for (auto idx = ref_dist_idx_array.begin(); idx != ref_dist_idx_array.end(); idx++) { | ||
| /// Compute the delta vector from the pattern center. | ||
| unsigned int ref_id = ref_id_array[*idx]; | ||
| double ref_delta[3]; |
There was a problem hiding this comment.
Make this an ndarray, so you can directly assign it:
Array<double, 1, 2> ref_delta = copy(reference_array[ref_id] - ref_ctr);
There was a problem hiding this comment.
Will do. However I will point out that the last number of is the number of contiguous dimensions. So you can't have 1, 2. This would be 1, 1.
| (proj_ref_delta[0] * proj_ref_ctr_delta[1] - proj_ref_delta[1] * proj_ref_ctr_delta[0]) / | ||
| geom_dist_ref; | ||
| double sin_theta_ref = | ||
| cross_ref[0] * ref_ctr[0] + cross_ref[1] * ref_ctr[1] + cross_ref[2] * ref_ctr[2]; |
There was a problem hiding this comment.
And again, with Eign, something like:
double sin_theta_ref = cross_ref.asEigenMatrix().dot(ref_ctr.asEigenMatrix());
There was a problem hiding this comment.
I'll give these a shot on one or two of them and see if I can get it to compile/run. If I can't though, I'm not too uncomfortable with just leaving it for now.
|
Clang format is running every time I save the file, yes. |
morriscb
force-pushed
the
tickets/DM-32008
branch
from
6234f6c
to
a2e0de3
Compare
morriscb
force-pushed
the
tickets/DM-32008
branch
3 times, most recently
from
c2f5f27
to
1dc498a
Compare
| namespace astrom { | ||
|
|
||
| int check_spoke(double cos_theta_src, double sin_theta_src, ndarray::Array<double, 1, 1> const& ref_ctr, | ||
| uint16_t ref_ctr_id, ndarray::Array<double, 1, 1> const& proj_ref_ctr_delta, |
There was a problem hiding this comment.
Hang on, I don't see ref_ctr_id used in the C++ code below at all?
| * the source spoke we are trying to test and the candidate reference | ||
| * spokes. | ||
| * @param[in] ref_id_array Array of id lookups into the master reference array | ||
| * that our center id object is paired with. |
There was a problem hiding this comment.
Please add a note here about why this is uint16 (as you said, the higher level code trims the arrays due to memory limits): it's generally expected that indexes are size_t.
|
Done! Take a look at the comment for yourself if you want. |
morriscb
force-pushed
the
tickets/DM-32008
branch
from
8392297
to
04221e1
Compare
Add pybind layer. Add pessimistic utils to SConscript and import. Add testSpokeCC to pessimitic matcher. Remove call to old _test_spoke method. Create header and source file. Move code to source file. Change to using ndarray. Add doxygen. Change function name. Change input arrays to const& Switch to eigen operations for cross and dot products. Update comment around for loop. Change to double slash. Change cross product to eigen. Remove unused ref_ctr_id variable. Add comment. Clang fortmating.
morriscb
force-pushed
the
tickets/DM-32008
branch
from
04221e1
to
8898f17
Compare
No description provided.