Eigen transforms #744
Eigen transforms #744
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #744 +/- ##
==========================================
+ Coverage 39.70% 39.71% +0.01%
==========================================
Files 135 135
Lines 22392 22393 +1
==========================================
+ Hits 8890 8893 +3
+ Misses 13502 13500 -2 ☔ View full report in Codecov by Sentry. |
| @@ -15,6 +15,7 @@ def test_gvec_to_xy_from_file(test_data_dir): | |||
| allow_pickle=True | |||
| ) | |||
| for obj in arr: | |||
| print(1) | |||
| m.def("make_binary_rot_mat", &makeBinaryRotMat, "Function that computes a rotation matrix from a binary vector"); | ||
| m.def("make_rot_mat_of_exp_map", &makeRotMatOfExpMap, "Function that computes a rotation matrix from an exponential map"); | ||
| m.def("makeOscillRotMat", &makeOscillRotMat, "Function that generates a collection of rotation matrices from two angles (chi, ome)"); | ||
| m.def("anglesToGvecToDetectorXYFromAngles", &anglesToGvecToDetectorXYFromAngles, "I hate this."); |
There was a problem hiding this comment.
This function could just be called anglesToDetectorXY instead.
There was a problem hiding this comment.
| m.def("anglesToGvecToDetectorXYFromAngles", &anglesToGvecToDetectorXYFromAngles, "I hate this."); | |
| m.def("anglesToDetectorXY", &anglesToDetectorXY, "Converts angles to detector xy coordinates."); |
Seems fine to me.
| src_files = [ | ||
| str(cpp_transform_pkgdir / 'src/transforms.cpp'), | ||
| str(cpp_transform_pkgdir / 'src/inverse_distortion.cpp'), | ||
| ] |
There was a problem hiding this comment.
| src_files = [ | |
| str(cpp_transform_pkgdir / 'src/transforms.cpp'), | |
| str(cpp_transform_pkgdir / 'src/inverse_distortion.cpp'), | |
| ] |
| inverse_distortion_ext = Extension( | ||
| name='hexrd.extensions.inverse_distortion', | ||
| sources=src_files, | ||
| extra_compile_args=compiler_flags+['-std=c++14'], | ||
| sources=[src_files[1]], |
There was a problem hiding this comment.
| sources=[src_files[1]], | |
| sources=[cpp_transform_pkgdir / 'src/inverse_distortion.cpp'], |
| @@ -123,15 +137,23 @@ def get_cpp_extensions(): | |||
| numpy.get_include(), | |||
| ] | |||
|
|
|||
| transforms_ext = Extension( | |||
| name='hexrd.extensions.transforms', | |||
| sources=[src_files[0]], | |||
There was a problem hiding this comment.
| sources=[src_files[0]], | |
| sources=[cpp_transform_pkgdir / 'src/transforms.cpp'], |
| auto mat22 = rMat_c(2, 2); | ||
|
|
||
| // SIMD loop for full batches | ||
| for (size_t i = 0; i < num_full_batches * batch_size; i += batch_size) { |
There was a problem hiding this comment.
Can we use xsimd::transform here to avoid writing this loop body twice?
There was a problem hiding this comment.
Lets not bother with this. Apparently xsimd::transform was removed recently and is currently in limbo.
xtensor-stack/xsimd#1045
There was a problem hiding this comment.
Theres some odd indentation all around this file.
Since we don't have a lot of cpp in this repo, I think its a good idea to start using clang-format here.
|
|
||
| # Two binary rmats should be the identity | ||
| assert np.allclose(rmat @ rmat, np.eye(3)) | ||
|
|
There was a problem hiding this comment.
| assert np.allclose(rmat.T @ rmat, np.eye(3)), "It is orthogonal" | |
| assert np.all((np.abs(rmat) - 1 < 1e-10) | (np.abs(rmat) < 1e-10)), "It is binary" |
These seem like reasonable tests to have as well,
| '-Wall', | ||
| '-shared', | ||
| '-std=c++14', | ||
| '-funroll-loops', |
There was a problem hiding this comment.
I think Windows is just going to straight up ignore these. Maybe we should write alternative flags to use for Windows, so it will be optimized as well? Like we did above on line 47. We do have Windows users.
|
I ran a quick performance check on running |
No description provided.