Planar #358
Planar #358
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #358 +/- ##
==========================================
+ Coverage 91.40% 91.43% +0.03%
==========================================
Files 72 73 +1
Lines 12659 12709 +50
==========================================
+ Hits 11571 11621 +50
Misses 1088 1088
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/simsoptpp/curveplanarfourier.cpp
Outdated
| for (int k = 0; k < numquadpoints; ++k) { | ||
| double phi = 2 * M_PI * quadpoints[k]; | ||
| for (int i = 0; i < order+1; ++i) { | ||
| data(k, 0) += rc[i] * cos(i*phi) * cos(phi); | ||
| data(k, 1) += rc[i] * cos(i*phi) * sin(phi); | ||
| } | ||
| } | ||
| for (int k = 0; k < numquadpoints; ++k) { | ||
| double phi = 2 * M_PI * quadpoints[k]; | ||
| for (int i = 1; i < order+1; ++i) { | ||
| data(k, 0) += rs[i-1] * sin(i*phi) * cos(phi); | ||
| data(k, 1) += rs[i-1] * sin(i*phi) * sin(phi); | ||
| } | ||
| } |
There was a problem hiding this comment.
Here and in the other subroutines in this file, these two k loops can be combined into a single loop. And if you separate out the i=0 case, then the i loops can be combined as well. Also I'd evaluate cos(i*phi), sin(i*phi), cos(phi), and sin(phi) just once rather than multiple times. Those trig functions can be the rate-limiting step.
| same normalized quaternion. Normalizing the dofs directly would create a | ||
| dependence between the quaternion dofs, which may cause issues during | ||
| optimization. | ||
|
|
There was a problem hiding this comment.
I'm not familiar with quaternions -- is there a computational advantage using them over a simple solid body rotation matrix + translation? You would have 3 dofs for the rotation (three rotation angles) and 3 dofs for the translation. If there is a computational advantage, can that be added to the docstring for the class? The formulas in curveplanarfourier.cpp on lines 43-45 are opaque to me.
| j = data(m, 1); | ||
| k = data(m, 2); | ||
|
|
||
| data(m, 0) = (i - 2 * (q_norm[2] * q_norm[2] + q_norm[3] * q_norm[3]) * i + 2 * (q_norm[1] * q_norm[2] - q_norm[3] * q_norm[0]) * j + 2 * (q_norm[1] * q_norm[3] + q_norm[2] * q_norm[0]) * k) + center[0]; |
There was a problem hiding this comment.
lines 43, 44, 45 are opaque to me. Do they complete the solid body rotation? can you comment the code here to explain, and maybe give a reference?
src/simsoptpp/curveplanarfourier.cpp
Outdated
| sinphi = sin(phi); | ||
| cosphi = cos(phi); | ||
| data(k, 0) = rc[0] * cosphi; | ||
| data(k, 1) = rc[0] * sinphi; | ||
| for (int i = 1; i < order+1; ++i) { | ||
| siniphi = sin(i * phi); | ||
| cosiphi = cos(i * phi); | ||
| data(k, 0) += (rc[i] * cosiphi + rs[i-1] * siniphi) * cosphi; | ||
| data(k, 1) += (rc[i] * cosiphi + rs[i-1] * siniphi) * sinphi; | ||
| } |
There was a problem hiding this comment.
Look at how I refactored this loop to minimize the number of calls to sin() and cos(). These transcendental functions are slow, so it is good to call them as few times as possible, by storing the results in cosphi, cosiphi, etc. Please apply a similar refactoring to the other functions in this file to reduce the number of calls to sin() and cos().
|
|
||
| #include "xtensor-python/pyarray.hpp" // Numpy bindings | ||
| typedef xt::pyarray<double> Array; | ||
| template class CurvePlanarFourier<Array>; |
There was a problem hiding this comment.
Are these lines needed, or can they be deleted?
There was a problem hiding this comment.
I'm not sure exactly what these lines do, but they are included in both of the other curve classes, so I thought it would be safest to include them.
Created a curve class for planar coils with a fourier series in polar coordinates, a quaternion based rotation vector, and a center vector in cartesian coordinates. This allows for planar coils to be constructed and used in optimization.