search

imcs-compsim / MIRCO

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

Deprecate Epetra #70

Closed mayrmt closed 1 year ago

mayrmt commented 1 year ago

Description and Context

Related Issues and Pull Requests

How Has This Been Tested?

Test suite not passing yet. Still WIP.

Checklist

Interested Parties / Possible Reviewers

@RShaw026 @NoraHagmeyer

mayrmt commented 1 year ago

Next steps

NoraHagmeyer commented 1 year ago

Next steps

  • [ ] Fix failing tests
  • [ ] Re-evaluate the use of specialized matrices and solvers to exploit symmetry and positive definiteness
  • [ ] (Maybe) use Teuchos::SerialDenseVector for vectors (instead of misusing Teuchos::SerialDenseMatrix with just one column)

Keeping up-to-date with developments in trilinos is a very important step since MIRCO heavily relies on it. Though, if the linear solver is changed to a shared-memory variant (#1) in the near future some of these steps will probably be overkill. As I was not privy to the last "MIRCO - next steps" discussion I can't speak to the plan going forward though. Maybe @isteinbrecher has some thoughts on that as well. In any case, this is a valuable contribution, and since the work is already done, my comment is probably not as helpful anymore. In light of #1, reevaluating the speed of the linear solver might not be a priority though.

NoraHagmeyer commented 1 year ago

Regarding the tests.. I can't say what fails.. but until now MIRCO has relied on the fact that the matrix is symmetric and the solver actually only reads one half of the matrix.. this might give you problems now

isteinbrecher commented 1 year ago

Looks good to me, definitely a good thing to get rid of Epetra. Regarding the Teuchos::SerialDenseVector, since this one inherits from Teuchos::SerialDenseMatrix the change should be straight forward.

mayrmt commented 1 year ago

Tests are passing after exploiting symmetry and positive definiteness as in the Epetra implementation. I also spent a couple of minutes to introduce some Teuchos::SerialDenseVectors for vectors.

NoraHagmeyer commented 1 year ago

Apparently, the workflow has been disabled for the last 3 months because of inactivity.. all tests should be regularly run and at the very least be run before merging. @RShaw026 Please make sure that the state of MIRCO is well-tested and this does not happen. I hope simply reinstating the workflow does the trick and MR are automatically tested again in the future.

mayrmt commented 1 year ago

My comment about failing tests was not related to automated testing, just to failing tests after I made changes to the code locally on my machine.

EDIT: In fact, I just was notified about an automated workflow run. So, automated testing seems to work.

mayrmt commented 1 year ago

I'm done so far with this branch. Please have a look! We can then decide if we merge it as is or if further changes make sense.

NoraHagmeyer commented 1 year ago

My comment about failing tests was not related to automated testing, just to failing tests after I made changes to the code locally on my machine.

EDIT: In fact, I just was notified about an automated workflow run. So, automated testing seems to work.

I know, it is now running again since I reenabled automatic testing. The tests are meant to be automatically run every time a MR is opened and during a daily scheduled pipeline. MRs are not meant to be approved and merged without a proper run of the workflow. Looking at your MR I was wondering why I couldn't find any workflow.

RShaw026 commented 1 year ago

Okay, thanks. I will keep that in mind.

@mayrmt This MR looks good to me.

RShaw026 commented 1 year ago

@maxfirmbach This PR solves the compile errors I was getting in BACI for using Epetra_SerialSymDenseMatrix and Epetra_SerialSpdDenseSolver in MIRCO. Therefore, merging this PR.