adding weights for C++ (#267) #268

joehellmersNOAA · 2025-12-22T19:40:30Z

What type of PR is this? (check all applicable)

Description

This pull request is needed to fix the C++ weights, and does so by separating them into two new classes that are not part of the associated operators.

Related Issues & Documents

Closes #
Algorithm for P and Q weights #224 P and Q weights in C++ divergence and gradient superfluous and confusing #216

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.

Added/updated tests?

_We encourage you to test all code included with MOLE, including examples.

Yes
No, and this is why: please replace this line with details on why tests
have not been included
I need help with writing tests

Read Contributing Guide and Code of Conduct

I have read and followed the Contributing Guide and the Code of Conduct

[optional] Are there any post deployment tasks we need to perform?

No

[optional] What gif best describes this PR or how it makes you feel?

jbrzensk

Yeah @joehellmersNOAA !!
I put some comments, mostly have to do with spacing, using the arma:: before Armadillo things, and const before the constant declaration in the test.

Good for 1D, I think that 2D and 3D are not as trivial as we think, hence the 2D in MATLAB, and no 3D implementations! I would leave the 2D and 3D until there is a better understanding, and a need, for them.

jbrzensk · 2025-12-22T20:45:22Z

src/cpp/weightsP.cpp

+  b.at(0) = -1.0;
+  b.at(m+1) = 1.0;
+
+


nit: extra lineIDK

jbrzensk · 2025-12-22T20:45:32Z

src/cpp/weightsP.cpp

+#include "gradient.h"
+#include "weightsP.h"
+
+


nit: extra line

jbrzensk · 2025-12-22T20:46:40Z

src/cpp/weightsQ.h

+
+};
+
+


nit; extra blank line

jbrzensk · 2025-12-22T20:57:59Z

tests/cpp/test6.cpp

+#include "mole.h"
+#include <gtest/gtest.h>
+
+vec Q_baseline[3];


jbrzensk · 2025-12-22T21:00:13Z

tests/cpp/test6.cpp

+
+vec Q_baseline[3];
+vec P_baseline[3];
+Real tolerance = 1.0e-08;


jbrzensk · 2025-12-22T21:02:20Z

tests/cpp/test6.cpp

+        // Baseline weights - Generated from MatLab/Octave
+        Q_baseline[0] = { 1.000000000,1.000000000,1.000000000,1.000000000,1.000000000,1.000000000,1.000000000};
+        Q_baseline[1] = { 1.000000000,1.125064293,0.751414447,1.162097097,0.962852897,0.997142531,0.962852897,
+                        1.162097097,0.751414447,1.125064293,1.000000000 };


nit; If you break the line, line up the numbers. And stay consistent. Some of the lines line up with the parentheses, others line up with the brackets. I vote to line up the numbers, it looks nicer.

jbrzensk · 2025-12-22T21:03:39Z

tests/cpp/test6.cpp

+        P_baseline[2] = { 0.315722926,1.390677390,0.629532984,1.234237262,0.919144328,1.009804636,
+                    1.000880474,1.000880474,1.009804636,0.919144328,1.234237262,0.629532984,
+                    1.390677390,0.315722926 };
+      for (int k : {2,4,6}) {


put a blank line before the for, for consistency

jbrzensk · 2025-12-22T21:04:06Z

src/cpp/weightsP.cpp

+{
+  Gradient G(k, m, dx);
+
+  vec b(m+2);


jbrzensk · 2025-12-22T21:04:16Z

src/cpp/weightsP.cpp

+  b.at(m+1) = 1.0;
+
+
+  sp_mat Gtranspose = G.t();


arma::sp_mat

jbrzensk · 2025-12-22T21:06:49Z

tests/cpp/test6.cpp

+      for (int k : {2,4,6}) {
+        run_Qtest(k);
+        run_Ptest(k);
+    }


For consistency, you put spaces between the closing loops and function in the previous Qweights and Pweights. Do the same here.

aboada

Thanks, still working on the review. I recommend applying clang-format (see: ClangFormat Documentation) to fix any formatting issues.

aboada · 2025-12-23T21:33:53Z

src/cpp/utils.cpp

 #include <cassert>

-#ifdef EIGEN
+//#ifdef EIGEN


Why are we commenting out this line? Is Eigen mandatory now?

aboada · 2025-12-23T21:37:54Z

src/cpp/utils.cpp

 }
-#endif
+
+vec Utils::spsolve_eigenQR(const sp_mat &A, const vec &b) {


Could you elaborate on why QR factorization is needed here as opposed to LU? I’m assuming this is for computing P and Q. Are we expecting any ill-conditioned matrices?

Moreover, the only difference between the LU method and the QR one is:

1c1 < vec Utils::spsolve_eigen(const sp_mat &A, const vec &b) { --- > vec Utils::spsolve_eigenQR(const sp_mat &A, const vec &b) { 4c4 < Eigen::SparseLU<Eigen::SparseMatrix<Real>, Eigen::COLAMDOrdering<int>> solver; --- > Eigen::SparseQR<Eigen::SparseMatrix<Real>, Eigen::COLAMDOrdering<int>> solver;

We should consider refactoring to avoid code duplication.

aboada · 2025-12-23T23:21:10Z

tests/cpp/test6.cpp

Could we give this test a more descriptive name? Since we may add more functionality in the future, how will we know that test6 is associated with P and Q?
Changing the test name may incur additional updates to the instructions in the CMakeList. If that's too much, we can do it in a separate PR.

aboada · 2025-12-23T23:25:12Z

tests/cpp/test6.cpp

+
+        for (int k : {2,4,6}) {
+            run_Qtest(k);
+            run_Ptest(k);


Unit tests should be independent, isolated, and ideally testing only one thing (focusing on a single responsibility). Could we split the tests for P and Q?

aboada · 2025-12-23T23:33:17Z

tests/cpp/test6.cpp

+
+TEST(WeightTests, Accuracy) {
+
+        // Baseline weights - Generated from MatLab/Octave


Instead of hardcoding these values from Matlab/Octave, why don't we check that $G^Tp = b_{m+2}$ (and the same for $Q$), as shown in Corbino and Castillo?

We can also have a separate test analogous to examples/matlab_octave/integration1D.m.

aboada · 2025-12-23T23:36:49Z

tests/cpp/test6.cpp

+            1.000880474,1.009804636,0.919144328,1.234237262,0.629532984,1.390677390,0.315722926
+        };
+
+        for (int k : {2,4,6}) {


To ensure robustness, we should also add tests that check for unexpected, unsupported, corner cases, etc.

For instance, what happens with negative k when calling the WeightsQ constructor (k,m,dx). What about odd values? Negative m or dx, etc.

joehellmersNOAA · 2025-12-24T01:41:41Z

Thanks, still working on the review. I recommend applying clang-format (see: ClangFormat Documentation) to fix any formatting issues.

I have no problem passing this C++ code through some sort of linter, but others should weigh in if this is the best one or not. @jbrzensk @cpaolini

adding weights for C++ (#267)

516730e

joehellmersNOAA requested review from aboada, jbrzensk and mdumett December 22, 2025 19:40

joehellmersNOAA assigned jbrzensk Dec 22, 2025

jbrzensk reviewed Dec 22, 2025

View reviewed changes

joehellmersNOAA added 2 commits December 22, 2025 23:16

fix nit

cac956e

fix nits

757f6e3

aboada reviewed Dec 23, 2025

View reviewed changes


		TEST(WeightTests, Accuracy) {

		// Baseline weights - Generated from MatLab/Octave

adding weights for C++ (#267) #268

Are you sure you want to change the base?

adding weights for C++ (#267) #268

Uh oh!

Conversation

joehellmersNOAA commented Dec 22, 2025

What type of PR is this? (check all applicable)

Description

Related Issues & Documents

QA Instructions, Screenshots, Recordings

Added/updated tests?

Read Contributing Guide and Code of Conduct

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

Uh oh!

jbrzensk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboada Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joehellmersNOAA commented Dec 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aboada Dec 23, 2025 •

edited

Loading