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

Make MassInvPC subclass AssembledPC #3372

Merged
merged 5 commits into from
Feb 7, 2024
Merged

Make MassInvPC subclass AssembledPC #3372

merged 5 commits into from
Feb 7, 2024

Conversation

pbrubeck
Copy link
Contributor

Description

Make MassInvPC subclass AssembledPC

@stephankramer
Copy link
Contributor

stephankramer commented Jan 30, 2024

Two comments:

  • This fixes an issue with MassInvPC not being updated when the value of mu changes between solve calls (update() currently doesn't do anything) - which is nice
  • This would change the options structure somewhat for the common case where an iterative solver is used because AssembledPC does not define a ksp directly under its prefix, it just does a pcapply. For example in tests/regression/test_nullspace.py
        'fieldsplit_1': {
            'ksp_type': 'fgmres',
            'pc_type': 'python',
            'pc_python_type': 'firedrake.MassInvPC',
            'Mp_ksp_type': 'cg',
            'Mp_pc_type': 'sor',
            'ksp_rtol': '1e-7',
            'ksp_monitor': None,
        }

would need to change to:

        'fieldsplit_1': {
            'ksp_type': 'fgmres',
            'pc_type': 'python',
            'pc_python_type': 'firedrake.MassInvPC',
            'Mp_pc_type': 'ksp',
            'Mp_ksp_ksp_type': 'cg',
            'Mp_ksp_pc_type': 'sor',
            'ksp_rtol': '1e-7',
            'ksp_monitor': None,
        }

(of the top of my head, untested) - so a little more unwieldy.

Copy link
Member

@dham dham left a comment

Choose a reason for hiding this comment

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

Fix tests and we're good.

@pbrubeck pbrubeck force-pushed the pbrubeck/cleanup-massinv branch from 1b85d20 to 517df4b Compare February 1, 2024 10:11
@dham dham merged commit fbad403 into master Feb 7, 2024
10 checks passed
@dham dham deleted the pbrubeck/cleanup-massinv branch February 7, 2024 16:15
stephankramer added a commit to g-adopt/g-adopt that referenced this pull request Apr 3, 2024
The main reason the have our own version of MassInvPC was that MassInvPC
did not update for varying (between solves) viscosities. This
has been fixed in firedrakeproject/firedrake#3372
and since then the options structure for the solvers beneath it has
changed: need to use pcksp with an additional ksp_ layer now. Thus
VariableMassInvPC is no longer a drop in-replacement for MassInvPC
as it was intended. Better to stay consistent with Firedrake therefore.
angus-g pushed a commit to g-adopt/g-adopt that referenced this pull request Apr 10, 2024
The main reason the have our own version of MassInvPC was that MassInvPC
did not update for varying (between solves) viscosities. This
has been fixed in firedrakeproject/firedrake#3372
and since then the options structure for the solvers beneath it has
changed: need to use pcksp with an additional ksp_ layer now. Thus
VariableMassInvPC is no longer a drop in-replacement for MassInvPC
as it was intended. Better to stay consistent with Firedrake therefore.
angus-g pushed a commit to g-adopt/g-adopt that referenced this pull request Apr 13, 2024
* Remove VariableMassInvPC

The main reason the have our own version of MassInvPC was that MassInvPC
did not update for varying (between solves) viscosities. This
has been fixed in firedrakeproject/firedrake#3372
and since then the options structure for the solvers beneath it has
changed: need to use pcksp with an additional ksp_ layer now. Thus
VariableMassInvPC is no longer a drop in-replacement for MassInvPC
as it was intended. Better to stay consistent with Firedrake therefore.
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.

3 participants