Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stefano's fixes and improvements #2

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stefanozampini
Copy link
Collaborator

@stefanozampini stefanozampini commented Aug 3, 2020

I will use this PR to provide my feedback to the project

First thing, you do not need to test for HYPRE support at configure time, PETSc provides a macro for it (PETSC_HAVE_HYPRE). In general, PETSC_HAVE_XXX indicates PETSc has been configured with support for XXX package. Similarly, PETSC_USE_COMPLEX indicates PETSc has been configure with PetscScalar == complex_number.
Do you want me to fix also the complex thing? Note that PETSC_HAVE_COMPLEX, just indicates that std::Complex is available to PETSc, see here https://gitlab.com/petsc/petsc/-/blob/master/include/petscsystypes.h#L208

@stefanozampini stefanozampini marked this pull request as draft August 3, 2020 13:27
@stefanozampini stefanozampini changed the title Stefanozampini/wip Stefano's fixes and improvements Aug 3, 2020
@stefanozampini
Copy link
Collaborator Author

Having few examples of usage from C++ would be nice, since the interface library is callable from C++. I can only find python examples in the project directory.

@stefanozampini
Copy link
Collaborator Author

Other initial comments:

  • what is the minimal version of PETSc you want to support?

  • I think you should not expose petsc.h in public headers. You can include petscsystypes.h for basic datatypes (eg PetscInt, PetscScalar, etc), and just forward declare the PETSc classes that are visible through the interface in src/typedefs.hpp, see e.g. https://github.com/mfem/mfem/blob/master/linalg/petsc.hpp#L42

@stefanozampini
Copy link
Collaborator Author

Pushed some more comments directly in the code 6e96003

@LukasKogler LukasKogler marked this pull request as ready for review August 4, 2020 08:26
@LukasKogler
Copy link
Contributor

Thank you for all your suggestions!

I merged your commit for the HYPRE support, this is really much cleaner!

what is the minimal version of PETSc you want to support?

I am not sure, we are discussing this. Do you have a suggestion? Do PETSc users tend to use older PETSc versions, or do they generally keep up with the releases?

Do you want me to fix also the complex thing?

I did it myself, thank you for the hint about PETSC_USE_COMPLEX!

I think you should not expose petsc.h in public headers

I changed this the way you suggested.

Having few examples of usage from C++ would be nice

I added a simple C++ example.

As for the comments in the code:

  • I still have trouble interfacing with a 64 bit index PETSc installation (linking issues).
  • The petsc_ops.hpp file is not used, I just forgot to remove it. Sorry for the inconvenience.
  • I put in the code you suggested in petsc_snes.cpp
  • SBAIJ support is indeed still missing
  • Using AIJ/MPIAIJ instead of BAIJ/MPIBAIJ per default might be a good idea. I have to write the functions to directly assemble AIJ matrices from our block-matrix format.
  • I am still experimenting with PCFIELDSPLIT (and MATNEST). Thanks for the warning!

Thanks again for your feedback, the inteface already looks much neater!

Best,
Lukas

@LukasKogler LukasKogler marked this pull request as draft August 5, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants