imcs-compsim / MIRCO

A shared-memory parallel BEM code for the contact of rough surfaces
MIT License
2 stars 3 forks source link

Fix bug by removing the need of copying Epetra matrix to std vector #46

Closed RShaw026 closed 2 years ago

RShaw026 commented 2 years ago

Description and Context

Earlier Epetra matrix output from the warm start function was copied to a std vector in the InitialGuessPredictor for further use in the NNLS. The program was crashing sometimes while copying, especially in the cases with more refined surfaces. Now, Epetra matrix is not copied to a std vector and is directly used as an input in the NNLS.

Related Issues and Pull Requests

How Has This Been Tested?

Tried running input files with high delta values and high resolution parameters (which were failing earlier), and they are running without an error. Also, Compared these results with the results of Matlab code and they are matching even when the tolerance value is set to 0.0001 on both, Matlab and C++ coded (The number of iterations to reach the solution was also same for Matlab and C++ codes).

Checklist

Additional Information

Interested Parties / Possible Reviewers

@NoraHagmeyer

RShaw026 commented 2 years ago

@NoraHagmeyer Please let me know if know of any cases where the program crashed earlier. I would try running them to see if the problem persists.

NoraHagmeyer commented 2 years ago

@NoraHagmeyer Please let me know if know of any cases where the program crashed earlier. I would try running them to see if the problem persists.

I am only aware of a storage problem with n=8, as reported in #6. If anyone had problems and did not report it in an issue, i don't know about it.

NoraHagmeyer commented 2 years ago

looks great. I'd suggest to hand in the maximum number of iterations via an input parameter and to throw an error if it did not converge. The second part can also be done in a next MR though.

RShaw026 commented 2 years ago

@NoraHagmeyer Please let me know if know of any cases where the program crashed earlier. I would try running them to see if the problem persists.

I am only aware of a storage problem with n=8, as reported in #6. If anyone had problems and did not report it in an issue, i don't know about it.

I tried with n=8 and it is working fine and faster than the MATLAB code for up to a Delta value of around 32. After that, I am getting an error terminate called after throwing an instance of 'std::bad_alloc'. As the Delta increases, the number of nodes in contact is also higher, which might be causing the storage issue.

NoraHagmeyer commented 2 years ago

@RShaw026 Is there a possibility to test the new changes, i.e. to design a test case that would have failed before but runs though now? This would ensure that noone will introduce a similar bug the in future.

NoraHagmeyer commented 2 years ago

@RShaw026 Is there a possibility to test the new changes, i.e. to design a test case that would have failed before but runs though now? This would ensure that noone will introduce a similar bug the in future.

If it is infeasible to design such a unit test, #43 becomes more urgent.

RShaw026 commented 2 years ago

@NoraHagmeyer It is possible to write such a test; I have to write a test for the Evaluate function. Since I know for which input values the test was failing before but runs through now, I can design the test for the Evaluate function which takes an input file name as input and outputs the force value.

NoraHagmeyer commented 2 years ago

@NoraHagmeyer It is possible to write such a test; I have to write a test for the Evaluate function. Since I know for which input values the test was failing before but runs through now, I can design the test for the Evaluate function which takes an input file name as input and outputs the force value.

You have basically described #43 :wink: It's good that we had the same idea though. This can be taken care of in a separate MR though.

NoraHagmeyer commented 2 years ago

@NoraHagmeyer It is possible to write such a test; I have to write a test for the Evaluate function. Since I know for which input values the test was failing before but runs through now, I can design the test for the Evaluate function which takes an input file name as input and outputs the force value.

You have basically described #43 wink It's good that we had the same idea though. This can be taken care of in a separate MR though.

@RShaw026 An alternative would be to pull the fixed-point iteration in evaluate.cpp l.83-140 into its own function. That should make it possible to test the whole fixed-point iteration without relying on an input file. This would still not be a unit test but rather an integration test since it tests the interaction of multiple code parts. But that would be a possibility for now.

RShaw026 commented 2 years ago

@NoraHagmeyer It is possible to write such a test; I have to write a test for the Evaluate function. Since I know for which input values the test was failing before but runs through now, I can design the test for the Evaluate function which takes an input file name as input and outputs the force value.

You have basically described #43 wink It's good that we had the same idea though. This can be taken care of in a separate MR though.

@RShaw026 An alternative would be to pull the fixed-point iteration in evaluate.cpp l.83-140 into its own function. That should make it possible to test the whole fixed-point iteration without relying on an input file. This would still not be a unit test but rather an integration test since it tests the interaction of multiple code parts. But that would be a possibility for now.

@NoraHagmeyer It will be very difficult to set up a topology matrix of size 33*33 (n = 5) manually for testing. So we have to read the topology from a file. But now that we have to read from a file anyway, I'd prefer reading the input XML file. But for that, we have to somehow copy the input files to the build directory. 😅

NoraHagmeyer commented 2 years ago

@NoraHagmeyer It is possible to write such a test; I have to write a test for the Evaluate function. Since I know for which input values the test was failing before but runs through now, I can design the test for the Evaluate function which takes an input file name as input and outputs the force value.

You have basically described #43 wink It's good that we had the same idea though. This can be taken care of in a separate MR though.

@RShaw026 An alternative would be to pull the fixed-point iteration in evaluate.cpp l.83-140 into its own function. That should make it possible to test the whole fixed-point iteration without relying on an input file. This would still not be a unit test but rather an integration test since it tests the interaction of multiple code parts. But that would be a possibility for now.

@NoraHagmeyer It will be very difficult to set up a topology matrix of size 33*33 (n = 5) manually for testing. So we have to read the topology from a file. But now that we have to read from a file anyway, I'd prefer reading the input XML file. But for that, we have to somehow copy the input files to the build directory. sweat_smile

If you follow #43 you do not have to copy any input or reference files to the build directory, since cmake knows where to find the source directory. Have a look at https://gitlab.lrz.de/baci/baci/-/blob/master/TestingFramework.cmake.