Skip to content

Add mesh.nullspace_rotations and stokes.petsc_use_nullspace - #105

Merged
lmoresi merged 3 commits into
developmentfrom
feature/mesh-nullspace-rebase
Apr 7, 2026
Merged

Add mesh.nullspace_rotations and stokes.petsc_use_nullspace#105
lmoresi merged 3 commits into
developmentfrom
feature/mesh-nullspace-rebase

Conversation

@lmoresi

@lmoresi lmoresi commented Mar 31, 2026

Copy link
Copy Markdown
Member

Summary

  • Add mesh.nullspace_rotations property: returns symbolic velocity fields for rigid-body rotation null modes. Empty for boxes/wedges, populated by mesh factories for closed surfaces (annulus: 1 mode, spherical shell: 3 modes).
  • Add stokes.petsc_use_nullspace convenience property: enables pressure nullspace and auto-populates velocity nullspace basis from mesh.nullspace_rotations in one call.
  • Mesh factories updated: Annulus, AnnulusWithSpokes, AnnulusInternalBoundary, DiscInternalBoundaries, SphericalShell, SphericalShellInternalBoundary, SegmentedSphericalShell, SegmentedSphericalBall, CubedSphere.

Rebased from feature/mesh-nullspace onto current development. Closes #102.

Original commits by @gthyagi.

Test plan

  • CI passes
  • annulus.nullspace_rotations returns [Matrix([-y, x])]
  • shell.nullspace_rotations returns 3 rotation matrices
  • stokes.petsc_use_nullspace = True auto-populates velocity basis

Underworld development team with AI support from Claude Code

Copilot AI review requested due to automatic review settings March 31, 2026 08:51
@lmoresi

lmoresi commented Mar 31, 2026

Copy link
Copy Markdown
Member Author

Not sure why these were left out - they were made on a separate branch and not added to the PR. This will fix issue #102

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds geometry-aware PETSc nullspace convenience support by exposing rigid-body rotation modes on meshes and providing a Stokes solver toggle to enable nullspace handling using those modes. This reduces manual setup for closed-surface Stokes problems (annulus/spherical shells).

Changes:

  • Introduces Mesh.nullspace_rotations (symbolic rigid rotation velocity modes) with a default empty basis.
  • Populates _nullspace_rotations in several mesh factory functions (annulus, spherical shell variants, segmented spherical variants, cubed sphere).
  • Adds SNES_Stokes_SaddlePt.petsc_use_nullspace convenience property to enable pressure nullspace and auto-populate velocity nullspace modes.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/underworld3/meshing/spherical.py Adds 3D rotation nullspace modes for spherical shell factories and cubed sphere.
src/underworld3/meshing/segmented.py Adds 3D rotation nullspace modes for segmented spherical shell/ball factories.
src/underworld3/meshing/annulus.py Adds 2D rigid rotation nullspace mode for annulus/disc factories.
src/underworld3/discretisation/discretisation_mesh.py Initializes _nullspace_rotations and exposes nullspace_rotations property on Mesh.
src/underworld3/cython/petsc_generic_snes_solvers.pyx Adds petsc_use_nullspace convenience property to configure Stokes nullspace modes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 267 to 271
class boundary_normals(Enum):
Lower = 11
Upper = 12
Centre = 1

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

boundary_normals is defined here but never assigned to new_mesh.boundary_normals before returning, so it is currently dead code and the mesh keeps boundary_normals=None. Either wire this up (with actual normal vector expressions, consistent with other mesh factories) or remove the unused enum to avoid confusion.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Pre-existing issue, not introduced by this PR. The boundary_normals pattern may be removed entirely now that Gamma_N is normalised (merged in #109). Leaving as-is for now.

Comment on lines 480 to 484
class boundary_normals(Enum):
Lower = 11
Internal = 12
Upper = 13
Centre = 1

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

boundary_normals is defined here but never assigned to new_mesh.boundary_normals before returning, so it is currently dead code and the mesh keeps boundary_normals=None. Either wire this up (with actual normal vector expressions) or remove the unused enum to avoid confusion.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Pre-existing issue, not introduced by this PR. The boundary_normals pattern may be removed entirely now that Gamma_N is normalised (merged in #109). Leaving as-is for now.

``petsc_velocity_nullspace_basis`` separately.
"""
return (self._petsc_use_pressure_nullspace
and len(self._petsc_velocity_nullspace_basis) > 0)

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

The getter currently returns True only when both pressure nullspace is enabled and at least one velocity mode is configured. This makes petsc_use_nullspace report False for valid pressure-only configurations (and for meshes with no rotation modes), which is surprising for a "use nullspace" flag.

Suggested change
and len(self._petsc_velocity_nullspace_basis) > 0)
or len(self._petsc_velocity_nullspace_basis) > 0)

Copilot uses AI. Check for mistakes.
Comment on lines +3267 to +3271
if value and hasattr(self.mesh, 'nullspace_rotations'):
modes = self.mesh.nullspace_rotations
if modes:
self.petsc_velocity_nullspace_basis = modes
elif not value:

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

When enabling, this setter only overwrites petsc_velocity_nullspace_basis if mesh.nullspace_rotations is non-empty; otherwise it leaves any previously configured velocity nullspace modes in place. That can lead to stale/incorrect modes carrying over between runs/meshes. Consider always syncing the velocity basis to mesh.nullspace_rotations (even if empty) and/or only auto-populating when the user hasn’t set a custom basis.

Suggested change
if value and hasattr(self.mesh, 'nullspace_rotations'):
modes = self.mesh.nullspace_rotations
if modes:
self.petsc_velocity_nullspace_basis = modes
elif not value:
if value:
if hasattr(self.mesh, 'nullspace_rotations'):
modes = self.mesh.nullspace_rotations
# Always sync the velocity nullspace basis to the mesh rotations
# (even if empty) to avoid stale modes carrying over between runs.
self.petsc_velocity_nullspace_basis = modes or ()
else:
# Mesh does not define rotations: clear any existing basis to
# prevent reuse of stale nullspace modes.
self._petsc_velocity_nullspace_basis = ()
else:
# Nullspace handling disabled: clear any velocity nullspace modes.

Copilot uses AI. Check for mistakes.
lmoresi added a commit that referenced this pull request Apr 2, 2026
- Getter: use 'or' instead of 'and' so pressure-only nullspace
  (no rotation modes) reports True correctly
- Setter: always sync velocity basis to mesh.nullspace_rotations
  when enabling (even if empty) to prevent stale modes from
  a previous mesh carrying over

Addresses Copilot review comments on PR #105.

Underworld development team with AI support from Claude Code
lmoresi added 3 commits April 7, 2026 15:26
Mesh factories now set _nullspace_rotations with symbolic velocity
fields for rigid-body rotation null modes:

- Annulus, AnnulusWithSpokes, AnnulusInternalBoundary, DiscInternalBoundaries:
  1 mode (z-rotation)
- SphericalShell, SphericalShellInternalBoundary, CubedSphere,
  SegmentedSphericalShell, SegmentedSphericalBall:
  3 modes (x, y, z rotation)
- Box, QuarterAnnulus, SegmentofAnnulus, SegmentofSphere:
  0 modes (walls break symmetry, default [] from base class)

Each mode is a SymPy Matrix velocity field in Cartesian coordinates.
The solver can project these onto its FE space for PETSc MatSetNullSpace.

Underworld development team with AI support from Claude Code
Setting stokes.petsc_use_nullspace = True enables:
  1. Constant-pressure nullspace (petsc_use_pressure_nullspace)
  2. Velocity rotation modes from mesh.nullspace_rotations

This auto-populates petsc_velocity_nullspace_basis from the mesh
geometry, so users don't need to construct rotation modes manually.

For annulus: 1 pressure + 1 rotation = 2 modes
For spherical shell: 1 pressure + 3 rotations = 4 modes
For box: 1 pressure + 0 rotations = 1 mode

Existing PR #91 tests pass unchanged.

Underworld development team with AI support from Claude Code
- Getter: use 'or' instead of 'and' so pressure-only nullspace
  (no rotation modes) reports True correctly
- Setter: always sync velocity basis to mesh.nullspace_rotations
  when enabling (even if empty) to prevent stale modes from
  a previous mesh carrying over

Addresses Copilot review comments on PR #105.

Underworld development team with AI support from Claude Code
@lmoresi
lmoresi force-pushed the feature/mesh-nullspace-rebase branch from 9f4fe72 to 3672199 Compare April 7, 2026 22:48
@lmoresi
lmoresi merged commit 4e6a988 into development Apr 7, 2026
1 check passed
@lmoresi
lmoresi deleted the feature/mesh-nullspace-rebase branch June 13, 2026 00:52
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