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

Refactor LA into native and PETSc part and add assigners #3659

Merged
merged 51 commits into from
Mar 6, 2025

Conversation

jhale
Copy link
Member

@jhale jhale commented Mar 5, 2025

Supersedes #3652 which made it apparent that dolfinx.la should have a dedicated dolfinx.la.petsc module.

This PR introduces API changes:

dolfinx.la.create_petsc_vector -> dolfinx.la.petsc.create_vector
dolfinx.la.create_petsc_vector_wrap -> dolfinx.la.petsc.create_vector_wrap

Note that dolfinx.fem.create_vector is now a very light wrapper over dolfinx.la.create_vector, perhaps bringing into question the need for the former.

jhale and others added 30 commits February 27, 2025 15:43
Co-authored-by: Jørgen Schartum Dokken <dokken92@gmail.com>
Co-authored-by: Jørgen Schartum Dokken <dokken92@gmail.com>
Co-authored-by: Jørgen Schartum Dokken <dokken92@gmail.com>
Co-authored-by: Jørgen Schartum Dokken <dokken92@gmail.com>
A Collection has no ordering - Sequence is correct.
Use np.inexact instead of np.floating
@jorgensd
Copy link
Member

jorgensd commented Mar 5, 2025

Couldn’t we place all PETSC related structures in dolfinx.petsc instead of having spread namespaces as nls,fem,la that has all to be explicitly imported in DOLFINx?

@jhale
Copy link
Member Author

jhale commented Mar 6, 2025

Couldn’t we place all PETSC related structures in dolfinx.petsc instead of having spread namespaces as nls,fem,la that has all to be explicitly imported in DOLFINx?

Well, yes, possibly, but not in this PR. But it's a huge API change. Let's discuss it on Slack.

Copy link
Contributor

@michalhabera michalhabera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The separation into fem.petsc and la.petsc reads very nice! And also follows the C++ structure.

Why are all these helpers (la.petsc.create_vector_wrap, la.petsc.create_vector, la.petsc.assign, fem.petsc.assign, ...) written in Python only? Is the argument that C++ users would not need/want such helpers?

@jhale
Copy link
Member Author

jhale commented Mar 6, 2025

Why are all these helpers (la.petsc.create_vector_wrap, la.petsc.create_vector, la.petsc.assign, fem.petsc.assign, ...) written in Python only? Is the argument that C++ users would not need/want such helpers?

I discussed this with @garth-wells - providing similar functionality for C++ users would be beneficial as interfacing SNES in particular is apparently "very hard". However, it's not clear we would want to write helpers in C++ using PETSc and then expose to Python, or just provide two good designs that suits C++ and Python via PETSc C and petsc4py, respectively.

Co-authored-by: Michal Habera <michal.habera@gmail.com>
@garth-wells
Copy link
Member

Why are all these helpers (la.petsc.create_vector_wrap, la.petsc.create_vector, la.petsc.assign, fem.petsc.assign, ...) written in Python only? Is the argument that C++ users would not need/want such helpers?

I discussed this with @garth-wells - providing similar functionality for C++ users would be beneficial as interfacing SNES in particular is apparently "very hard". However, it's not clear we would want to write helpers in C++ using PETSc and then expose to Python, or just provide two good designs that suits C++ and Python via PETSc C and petsc4py, respectively.

There is a helper at

std::vector<std::vector<PetscScalar>> get_local_vectors(
.

More C++ 'helpers' could be helpful, but we should do it with a purpose, e.g. when adding SNES helpers, to get it right rather than adding it for hypothetical reasons.

@jhale jhale enabled auto-merge March 6, 2025 19:00
@jhale jhale added this pull request to the merge queue Mar 6, 2025
Merged via the queue into main with commit 4627a17 Mar 6, 2025
25 of 26 checks passed
@jhale jhale deleted the jhale/refactor-la-petsc branch March 6, 2025 19:46
jorgensd added a commit to jorgensd/dolfinx_mpc that referenced this pull request Mar 7, 2025
jorgensd added a commit to jorgensd/dolfinx_mpc that referenced this pull request Mar 7, 2025
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.

4 participants