Tickets/DM-13806 #53
Tickets/DM-13806 #53
Conversation
| using Moments = Eigen::Matrix<double, 6, 1>; | ||
| using Jacobian = Eigen::Matrix<double, 6, 6>; | ||
| using FirstMoment = Eigen::Matrix<double, 2, 1>; | ||
| using SecondMoment = Eigen::Matrix<double, 2, 2>; |
There was a problem hiding this comment.
You may want to add using Element = double; and then parameterize the rest on it.
| * maximally likely image moments given moments measured from an image. As such the class | ||
| * provides two methods in addition to the at method already mentioned. One method computes the | ||
| * biased weighted moments given the specified weight and "true" moments, and the other computes | ||
| * the derivative of the objective function along each of the image moment variables. |
There was a problem hiding this comment.
Maybe not start every sentence with "This class".
There was a problem hiding this comment.
Made this easier to read
| * moments intrinsic to an object, and a vector of moments representing a weighting function. | ||
| * | ||
| * The class is constructed with the moments of weight function, and the moments of the object | ||
| * are set by calling the at method. This allows a single instance to be reused to evaluate |
There was a problem hiding this comment.
switched name to setParameters
| * The class is constructed with the moments of weight function, and the moments of the object | ||
| * are set by calling the at method. This allows a single instance to be reused to evaluate | ||
| * multiple positions in object moment space. This structure is computationally more efficient | ||
| * as well since the weight function would remain constant for many evaluation of image moments |
| * a normalized weight function. | ||
| */ | ||
| MomentsModel(Moments const & W): W(W) { | ||
| } |
There was a problem hiding this comment.
Please also explicitly default or delete copy/move constructors / assignment operators and destructor.
src/RegularizedMoments.cc
Outdated
| // Calculates and returns the derivatives for the alpha components. | ||
| auto makeAlphaGrad(Moments const & Q, Moments const & W) { | ||
| double muBottom, sigBottom; | ||
| std::tie(muBottom, sigBottom) = makeAlphaGradDivisors(Q, W); |
src/RegularizedMoments.cc
Outdated
| if (abs(zeroTruth - zeroRes) > tol) { | ||
| return false; | ||
| } | ||
| return approxEqual(firstRes, firstTruth, tol); |
There was a problem hiding this comment.
return (abs(zeroTruth - zeroRes) < tol) && approxEqual(firstRes, firstTruth, tol)? Or move the first test higher to prevent the assignment cost? Or just leave these as they are, since they are minor tests anyway.
There was a problem hiding this comment.
not worth worrying about in the test that will get called a small number of time, and with small length vectors
| return false; | ||
| } | ||
| return approxEqual(firstRes, firstTruth, tol); | ||
| } |
There was a problem hiding this comment.
Again, i'd move all these tests out of here. Since they obscure the implementation of the class.
src/RegularizedMoments.cc
Outdated
|
|
||
| // Sets the location in moment space to evaluate | ||
| void MomentsModel::at(Moments const & inputQ) { | ||
| Q = Moments(inputQ); |
src/RegularizedMoments.cc
Outdated
| // Returns the value of the weighted biased moments calculation | ||
| Moments MomentsModel::computeValues() { | ||
| return value; | ||
| } |
There was a problem hiding this comment.
This doesn't compute anything. Also why is value singular and computeValues plural? Perhaps getValue would be better?
There was a problem hiding this comment.
change value to plural. Also this signature was chosen so in the future someone my substitute a different objective function where the work may be done in the computeValues function, and the api will stay the same
Add a class which functions as an objective functional model of biased weighted moments and its derivatives given input moments and weights. This is intended to be used in conjunction with an optimizer to find the moments which maximize the likelyhood of moments given input measurements from data.
No description provided.