Conversation
|
I noticed that the |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a typo in the spherical coefficient read for the HighFive Eigen3 workaround. The adjustments ensure that the spherical coefficients are divided by the square root of the corresponding norm values, and also correct the inner loop declaration in the coefficient reshuffling.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| exputil/OrthoFunction.cc | Applies the fix by dividing the spherical coefficients by sqrt(norm[n]). |
| expui/Coefficients.cc | Removes an erroneous inner loop variable reinitialization to correctly accumulate data. |
Comments suppressed due to low confidence (1)
expui/Coefficients.cc:134
- The removal of 'n=0' in the inner loop prevents the unintended resetting of the 'n' counter. Confirm that using the outer loop's counter for iterating through in2.data() is the intended behavior.
for (size_t r=0; r<in.rows(); r++) {
| @@ -114,7 +114,7 @@ void OrthoFunction::dumpOrtho(const std::string& filename) | |||
| // | |||
| Eigen::VectorXd p = poly_eval(y, nmax) * W(r); | |||
| fout << std::setw(16) << r; | |||
There was a problem hiding this comment.
[nitpick] Consider adding a comment explaining the rationale behind dividing by sqrt(norm[n]) to ensure that future maintainers understand the fix for the spherical coefficient read.
| fout << std::setw(16) << r; | |
| fout << std::setw(16) << r; | |
| // Normalize the polynomial coefficients by dividing by sqrt(norm[n]) | |
| // to ensure they adhere to the expected orthogonality or normalization properties. |
michael-petersen
left a comment
michael-petersen
left a comment
There was a problem hiding this comment.
This looks good to me. Nice bug catch, and the norm is appropriate.
I'm ambivalent about adding the copilot-suggested comment; it is possible we'll help ourselves in the future by including it, though.
|
Never hurts to add a comment. I'll do it. |
This very minor commit fixes a typo in the spherical coefficient read for the HighFive Eigen3 workaround. The cylindrical coefficient read does not have the typo.