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

Emmarothwell1/restricted _function_space: Implement a RestrictedFunctionSpace class. #3215

Merged
merged 152 commits into from
Apr 26, 2024

Conversation

emmarothwell1
Copy link
Contributor

Implement a RestrictedFunctionSpace class.

(Part of an M4R project)
This inherits from FunctionSpace, with the main difference being how boundary conditions are treated.
This will hopefully be used in cases where users have boundary conditions they want to apply.

@emmarothwell1 emmarothwell1 force-pushed the emmarothwell1/restricted_function_space branch 2 times, most recently from 33079b1 to e543a5d Compare December 5, 2023 15:15
@ksagiyam
Copy link
Contributor

In serial lgmap is not aware of the changes you have made in dmcommon.make_global_numbering:
https://github.com/OP2/PyOP2/blob/d017d594e0bda694a71c4180fbfeae1cba473e93/pyop2/types/dataset.py#L122.
I think we can just delete self.comm.size == 1 case.

@ksagiyam
Copy link
Contributor

With the above changes, I was able to get a 4 x 4 matrix with nonzeros only on the top-left 2 x 2 submatrix; this indicates that your lgmap is correct. The problem now reduces to getting a matrix of the right shape.
The allocation of a matrix is done in assemble.py using op2.Sparsity. The actual assembly is done in the same file using op2.Parloop.
When allocating, the size of the matrix is set here https://github.com/OP2/PyOP2/blob/d017d594e0bda694a71c4180fbfeae1cba473e93/pyop2/sparsity.pyx#L140, where rset.size is rset.set.size, which is the size of core + owned DoFs of the given rank. Here, rset.set is V.node_set. As we now want the size of the reduced matrix here, we probably want V.node_set to store the size of the "constrained owned DoFs" as, say, V.node_set.constrained_size, when we make node_set here

node_set = op2.Set(node_classes, halo=halo, comm=mesh.comm)
.
The computation of the size of the "constrained owned DoFs" should probably be done when we compute renumbering to push "constrained owned DoFs" to the end of core + owned block.

We currently only renumber (reorder) entities so that a local vector sees core, owned, and ghost DoFs in this order; we compute renumbering using dmcommon.plex_renumbering

def plex_renumbering(PETSc.DM plex,
when we construct a mesh, and set that to a section at https://github.com/firedrakeproject/firedrake/blob/8d97a0741907a54cfea1aa48a1ec4903f11756b3/firedrake/cython/dmcommon.pyx#L1231C41-L1231C41 so that PetscSectionGetOffset() would return offsets in the local vector as we want.
As we now want to push "constrained owned DoFs" to the end of the core + owned block,
when dmcommon.create_section is called with boundary_set, we need to compute renumbering using dmcommon.plex_renumbering (with additional argument boundary_set) instead of using the one stored in the mesh. When we do this, we can also compute the size of the constrained owned DoFs.

@emmarothwell1 emmarothwell1 force-pushed the emmarothwell1/restricted_function_space branch 2 times, most recently from 1492321 to 0c96508 Compare December 12, 2023 17:08
@ksagiyam
Copy link
Contributor

You should also push your PyOP2 branch and do something exactly like this https://github.com/firedrakeproject/firedrake/pull/3288/files for CI to pick up your PyOP2 branch.

@ksagiyam
Copy link
Contributor

Some PRs that will conflict with yours have just been merged, so you will need to rebase when you have time. Specifically, there has been a change in functionspaceimpl.py. I think the easiest way to deal with this is to first respect all changes in functionspaceimpl.py on master and then cherry pick/apply your changes on top of that, or just do it in your favorite way.

@emmarothwell1 emmarothwell1 force-pushed the emmarothwell1/restricted_function_space branch from e6412c1 to d530859 Compare December 29, 2023 18:52
Copy link
Contributor

@ksagiyam ksagiyam left a comment

Choose a reason for hiding this comment

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

You need to rebase to make CI running.

As for PyOP2, this PR OP2/PyOP2#714 has made Mat and Sparsity inherit sizes from Dataset.layout_vec, which represents corresponding vectors, so, after rebasing, you should only need to shrink the size of Dataset.layout_vec.

You will also need to run make in PyOP2, just as you do in Firedrake after changing Cython file. Before running make in PyOP2, you will need to:

export PETSC_DIR=$VIRTUAL_ENV/src/petsc
export PETSC_ARCH=default

.

firedrake/cython/dmcommon.pyx Outdated Show resolved Hide resolved
firedrake/cython/dmcommon.pyx Outdated Show resolved Hide resolved
firedrake/cython/dmcommon.pyx Outdated Show resolved Hide resolved
firedrake/cython/dmcommon.pyx Show resolved Hide resolved
@emmarothwell1 emmarothwell1 force-pushed the emmarothwell1/restricted_function_space branch from cdde2d0 to 1c1e67f Compare January 10, 2024 10:32
@emmarothwell1 emmarothwell1 force-pushed the emmarothwell1/restricted_function_space branch 2 times, most recently from 132ac13 to c7a572d Compare January 25, 2024 11:10
@ksagiyam
Copy link
Contributor

Cache issue has been sorted out #3355, so rebase.

@emmarothwell1 emmarothwell1 force-pushed the emmarothwell1/restricted_function_space branch from c7a572d to 5e21dc7 Compare January 27, 2024 18:01
@emmarothwell1 emmarothwell1 force-pushed the emmarothwell1/restricted_function_space branch from 94af4e8 to 438628f Compare February 15, 2024 17:58
@emmarothwell1 emmarothwell1 force-pushed the emmarothwell1/restricted_function_space branch from ed5a318 to 2035349 Compare February 26, 2024 15:32
@emmarothwell1 emmarothwell1 force-pushed the emmarothwell1/restricted_function_space branch from c900640 to edf9adc Compare April 17, 2024 14:14
firedrake/adjoint_utils/variational_solver.py Outdated Show resolved Hide resolved
firedrake/cython/dmcommon.pyx Outdated Show resolved Hide resolved
firedrake/functionspacedata.py Outdated Show resolved Hide resolved
firedrake/functionspacedata.py Outdated Show resolved Hide resolved
firedrake/functionspaceimpl.py Outdated Show resolved Hide resolved
firedrake/functionspaceimpl.py Show resolved Hide resolved
firedrake/functionspaceimpl.py Outdated Show resolved Hide resolved
tests/regression/test_restricted_function_space.py Outdated Show resolved Hide resolved
tests/regression/test_restricted_function_space.py Outdated Show resolved Hide resolved
@ksagiyam
Copy link
Contributor

Also please run make lint and fix lint failures.

firedrake/assemble.py Outdated Show resolved Hide resolved
tests/regression/test_restricted_function_space.py Outdated Show resolved Hide resolved
tests/regression/test_restricted_function_space.py Outdated Show resolved Hide resolved
tests/regression/test_restricted_function_space.py Outdated Show resolved Hide resolved
tests/regression/test_restricted_function_space.py Outdated Show resolved Hide resolved
tests/regression/test_restricted_function_space.py Outdated Show resolved Hide resolved
tests/regression/test_restricted_function_space.py Outdated Show resolved Hide resolved
firedrake/functionspaceimpl.py Outdated Show resolved Hide resolved
firedrake/assemble.py Outdated Show resolved Hide resolved
@emmarothwell1 emmarothwell1 dismissed ksagiyam’s stale review April 25, 2024 22:16

The merge-base changed after approval.

ksagiyam
ksagiyam previously approved these changes Apr 25, 2024
@ksagiyam ksagiyam marked this pull request as ready for review April 26, 2024 21:35
@ksagiyam ksagiyam merged commit 02bdb5f into master Apr 26, 2024
9 checks passed
@ksagiyam ksagiyam deleted the emmarothwell1/restricted_function_space branch April 26, 2024 21:43
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.

5 participants