search

openmopac / mopac

Molecular Orbital PACkage
http://openmopac.net
GNU Lesser General Public License v3.0
115 stars 31 forks source link

adding changes to cosmo #137

Closed QuChem closed 1 year ago

QuChem commented 1 year ago

Status

godotalgorithm commented 1 year ago

This is causing some tests to crash and fail in a manner that suggests memory allocation issues. Based on the size of the matrix being passed into coscl1, I am assuming that the matrix is being stored and operated on in a packed triangular matrix format (i.e. only about half of the matrix is being stored). dpotrf does not use packed matrix storage, so either the allocation and indexing of the matrix need to be updated for full-matrix storage, or it needs to be temporarily unpacked into full-matrix format for dpotrf with the matrix output repacked.

godotalgorithm commented 1 year ago

Actually, there appears to be a third option - LAPACK has a subroutine for Cholesky factorization in packed matrix format, dpptrf. I'm not sure if this version of the solver benefits as much from blocking and threading, but it would allow us to preserve the existing format for amat for now.

codecov-commenter commented 1 year ago

Codecov Report

Merging #137 (b736712) into main (7e4b992) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage   66.67%   66.67%           
=======================================
  Files         331      331           
  Lines       73757    73714   -43     
=======================================
- Hits        49180    49152   -28     
+ Misses      24577    24562   -15
Impacted Files Coverage Δ
src/solvation/cosmo.F90 90.59% <100.00%> (-0.01%) :arrow_down:
src/input/myword.F90 97.36% <0.00%> (-2.64%) :arrow_down:
src/input/getdat.F90 42.91% <0.00%> (-1.67%) :arrow_down:
src/matrix/ddiag.F90 98.60% <0.00%> (-1.40%) :arrow_down:
src/run_mopac.F90 76.34% <0.00%> (-1.40%) :arrow_down:
src/geometry/dihed.F90 45.94% <0.00%> (-1.36%) :arrow_down:
src/INDO/scf.F90 80.70% <0.00%> (-0.88%) :arrow_down:
src/MOZYME/hybrid.F90 80.91% <0.00%> (-0.77%) :arrow_down:
src/chemistry/names.F90 67.60% <0.00%> (-0.71%) :arrow_down:
src/INDO/ovlp.F90 89.77% <0.00%> (-0.67%) :arrow_down:
... and 28 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

godotalgorithm commented 1 year ago

dpptrf has comparable performance to dpotrf in MKL, but not in other LAPACK libraries that I tested (presumably OpenBLAS). Since the main distributions of MOPAC use MKL, it's fine to stick with dpptrf and the packed storage for now. Ideally, MOPAC should move away from packed storage in the future.

Also, after some testing, it appears that all of the diagonal elements of the Cholesky factor have been inverted for some reason. The Cholesky factor is used by coscl2 to solve an SPD linear system, so presumably this pre-inversion helps with that somehow.

While I welcome overhauls to the linear algebra in MOPAC, this example highlights the challenges in doing so - heavy use of packed triangular matrix storage, and weird, non-standard stuff in unexpected places (e.g. the inversion of the diagonals in the Cholesky decomposition). Also, my current tests are mainly for code logic - they aren't precisely comparing numerical outputs because the numerical precision of MOPAC is too loose to test numerics reliably. I do report observed numerical differences in tests, but they are only warnings and must be inspected by hand.