GridOPTICS / GridPACK

https://www.gridpack.org/
47 stars 22 forks source link

Fix ga_matrix.cpp #160

Closed bjpalmer closed 1 year ago

bjpalmer commented 1 year ago

ga_matrix.cpp is throwing a compilation error when building with newer versions of PETSc (3.18). The offending line is 892

https://github.com/GridOPTICS/GridPACK/blob/985fd5d73b338a32f55f24e845502797e437f48a/src/math/petsc/ga_matrix.cpp#L891C1-L900C100

The symbol MATOP_MAT_MULT is no longer defined in later versions of PETSc. A temporary kluge is to comment this line out but that may break something if your code is using dense matrices.

abhyshr commented 1 year ago

Using shell matrix operations is a very niche usage of PETSc that allows operations on user defined matrices. For most applications, the native PETSc matrices are sufficient. Is there some special usage in GridPack for which PETSc matrices cannot be used? In my opinion, we should avoid use of PETSc shell matrix.

bjpalmer commented 1 year ago

This code was put in so that we could support dense matrix operations needed by the Kalman filter application. The original attempts to build PETSc using the Elemental library were a complete failure.

wperkins commented 1 year ago

I would change MATOP_MAT_MULT to MATOP_MULT. I think that's equivalent.

Using shell matrix operations is a very niche usage of PETSc that allows operations on user defined matrices. For most applications, the native PETSc matrices are sufficient. Is there some special usage in GridPack for which PETSc matrices cannot be used? In my opinion, we should avoid use of PETSc shell matrix.

This was done because, at some time in the past, we needed a parallel dense matrix, for some reason. The PETSc shell matrix was used to define a dense matrix stored in a Global Array. AFAIK, there is still no option for a parallel dense matrix. When funding allows, the need for a parallel dense matrix and existing code to meet that need should be closely examined and some decision about keeping it made.

wperkins commented 1 year ago

Upon deeper examination, the GA-based matrix was used only to do a parallel dense matrix multiply. PETSc 3.14 and later supports parallel matrix multiply, so I vote to remove it from GridPACK and require PETSc 3.14 or newer. Changes in #164 makes this happen and should close this when merged.

wperkins commented 1 year ago

No longer applicable.