tofstie commented 1 month ago

Multicore Non-Negative Least Squares Code and Tests

Pull request for the addition of allowing the Non-Negative Least Squares (NNLS) to be run on multiple cores.

NNLS_solver.cpp

The input matrix and vector can now exist within multiple cores
The output matrix will now have the map and distribution of A.DomainMap()
All calculations will still happen on one core, but this adds the functionality of running NNLS with more than 1 core, speeding up other areas of the code.

helper_functions.cpp

Fixed the epetra_to_eig_vec function to properly output the Eigen::VectorXd to all cores
Added eig_to_epetra_vec function to output an Epetra_Vector from data in a Eigen::VectorXd

2D_inviscid_isentropic_vortex.prm

Corrected the interpolate initial condition parameter

NNLS_tests.cpp

Implemented new helper_function
Added new test for multicore NNLS

tofstie commented 1 month ago

Here are the failed tests from running ctest: LastTestsFailed.log All of them exist in the Issues tab under the label testfail

tofstie commented 1 month ago

Thanks for the PR, multicore functionality will be very useful for the ROM part of PHiLiP!

I've left a few comments which should be quick to resolve. Could you also confirm whether the result is the same when you run a test on a single processor vs. multiple processors?

The results are the same for a single processor and multiple processors. I will use Case 1 in the unit tests as an example.

Single Core

Epetra::Vector\ Solution x\ MyPID GID Value\ 0 0 0.1\ 0 1 0.5\

2 Cores

Epetra::Vector\ Solution x\ MyPID GID Value\ 0 0 0.1\ 1 1 0.5\ The only difference in the two vectors is where the values are stored. This is because there is no change to the logic of the NNLS solver, just that the input and output can be Epetra objects that are distributed over multiple cores.

cpethrick commented 1 month ago

Thanks for responding to my comments!

Regarding the question about the solution being the same on different numbers of cores, it's something to look out for as you increase the complexity of problems. We've seen it before where there were differences of ~10^-14 when running on different numbers of cores, but I am not sure whether that is in the most recent version of master or not. I'm glad that you get the same result for your unit test!

Approving, we can merge it in once @PranshulThakur and @biondicc have looked it over.

tofstie commented 1 month ago

Thanks for responding to my comments!

Regarding the question about the solution being the same on different numbers of cores, it's something to look out for as you increase the complexity of problems. We've seen it before where there were differences of ~10^-14 when running on different numbers of cores, but I am not sure whether that is in the most recent version of master or not. I'm glad that you get the same result for your unit test!

Approving, we can merge it in once @PranshulThakur and @biondicc have looked it over.

Just to make sure that there were no difference, I have tried the Matlab test case for 1 core and 2 core, which returns a vector of size 1024. I loaded the results into matlab, ensuring that the results are within std::numeric_limits<double>::digits10, and the difference of the vectors was exactly the zero vector. If you want to convince yourself, here are the results for 1 Core and 2 Core, with the second number being the rank of the processor. 2Corex,0.txt 2Corex,1.txt 1Corex,0.txt I also tried between 1 core and 8 core and again the difference had a norm of 0. Here is the matlab file if you want to try yourself comparing_multicore_NNLS.zip

tofstie commented 1 month ago

Thanks @tofstie for addressing all my comments. Can you try running make doc 2> output_error_doc.log in the build directory and see if there aren't any doxygen warnings from the functions you have implemented? The PR looks good to me but I will leave it to @biondicc to review and provide the final approval.

There are no doxygen warnings from the functions I have implemented. The only warning for the NNLS area is the following /home/tyson/Codes/HYPER_PHiLiP/src/linear_solver/NNLS_solver.h:71: warning: Compound PHiLiP::NNLS_solver is not documented. However, there is documentaiton, but not right above the NNLS. I'll move some of the documentation to the correct spot to ensure this error goes away. I will wait to commit to see if Calista has any issues first.

cpethrick commented 1 month ago

Thanks for responding to my comments! Regarding the question about the solution being the same on different numbers of cores, it's something to look out for as you increase the complexity of problems. We've seen it before where there were differences of ~10^-14 when running on different numbers of cores, but I am not sure whether that is in the most recent version of master or not. I'm glad that you get the same result for your unit test! Approving, we can merge it in once @PranshulThakur and @biondicc have looked it over.

Just to make sure that there were no difference, I have tried the Matlab test case for 1 core and 2 core, which returns a vector of size 1024. I loaded the results into matlab, ensuring that the results are within std::numeric_limits<double>::digits10, and the difference of the vectors was exactly the zero vector. If you want to convince yourself, here are the results for 1 Core and 2 Core, with the second number being the rank of the processor. 2Corex,0.txt 2Corex,1.txt 1Corex,0.txt I also tried between 1 core and 8 core and again the difference had a norm of 0. Here is the matlab file if you want to try yourself comparing_multicore_NNLS.zip

Sounds good! Just wanted to make sure this was the case :)