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

Allow PETSc options to be passed from BOUT.inp #1795

Merged
merged 30 commits into from
May 26, 2020
Merged

Conversation

johnomotani
Copy link
Contributor

This is a new feature, so shouldn't really go into v4.3, but it's quite important to me, and being forced to pass options to PETSc via the command line is a bit of a bug...

This PR allows setting arbitrary PETSc options in the BOUT.inp input file. Options can be set 'globally' in a [petsc] section (implemented in the first commit, if the rest of it is too much to add right away), or in subsections for specific solvers, e.g. [laplacexy:petsc], so that if there are multiple PETSc solvers they can be passed different options.

Arbitrary options can be passed: take the command line option for PETSc; remove the initial hyphen; if the option is a flag that doesn't take a value, pass true. So for example command line options

-ksp_monitor -ksp_type gmres

could be replaced with

[petsc]
ksp_monitor = true
ksp_type = gmres

I started on this because I'm having difficulty with slow convergence of LaplaceXY for some simulations I'm doing. I'm using hypre as a preconditioner and wanted to try changing settings to see if that helped. In the PETSc documentation https://www.mcs.anl.gov/petsc/petsc-current/docs/manualpages/PC/PCHYPRE.html

Apart from pc_hypre_type (for which there is PCHYPRESetType()), the many hypre options can ONLY be set via the options database (e.g. the command line or with PetscOptionsSetValue(), there are no functions to set them)

So I wanted a generic way to pass options to PetscOptionsSetValue(). Then I realised that it would not be too difficult to add options specific to a particular PetscLib object, and so to a particular solver object, by adding a unique prefix to the name of the PETSc option, using KSPSetOptionsPrefix(). I don't have a use for this yet, but it seems potentially a very useful feature if someone ever wants to use multiple PETSc-based solvers.

While I was adding a section to the manual, I noticed that the 'Invertable operator' section is missing from the index/table-of-contents, so there's a one-line commit adding that, which didn't seem worth a separate PR.

@johnomotani johnomotani added this to the BOUT-4.3 milestone Sep 24, 2019
Previously passed pointer to ksp instead of ksp.
@d7919
Copy link
Member

d7919 commented Sep 24, 2019

I certainly support the use of the option prefix, I've been meaning to implement that for a long time. Thanks for adding the invertable operator to the manual as well.

src/sys/petsclib.cxx Outdated Show resolved Hide resolved
src/sys/petsclib.cxx Outdated Show resolved Hide resolved
src/sys/petsclib.cxx Outdated Show resolved Hide resolved
src/sys/petsclib.cxx Outdated Show resolved Hide resolved
include/bout/petsclib.hxx Outdated Show resolved Hide resolved
@ZedThree
Copy link
Member

Thanks @johnomotani ! I've come up against needing to set PETSc options before and only being able to set them on the command line was a bit limiting.

This sort of thing also just makes me wish for a proper C++ interface to PETSc.

include/bout/petsclib.hxx Outdated Show resolved Hide resolved
include/bout/petsclib.hxx Outdated Show resolved Hide resolved
include/bout/petsclib.hxx Outdated Show resolved Hide resolved


.. [*] The object-specific options are passed to PETSc by creating an object-specific
prefix ``boutpetsclib#_``, where ``#`` is replaced with an integer counter,
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see -- this sounds like it might be tricky to be sure of. Could we provide a way to set the specific prefix to use for a particular instance of something in the object constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered about that, but wasn't sure if anyone would actually use it... I think it should be straightforward to do, we can add an optional argument to the constructor, so the user can set a name.

Copy link
Member

@ZedThree ZedThree Sep 24, 2019

Choose a reason for hiding this comment

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

I don't think we need to do anything necessary special here, we should just be able to use the usual BOUT++ mechanisms.

That is, instead of doing

./model -petsc_option

we can now do

./model laplacexy:petsc:petsc_option=value

More verbose, sure, but also more specific.

Copy link
Member

Choose a reason for hiding this comment

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

This footnote can go now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 good point. I'm testing out the current version now. Will update the manual before calling this PR ready to merge.

src/sys/petsclib.cxx Outdated Show resolved Hide resolved
src/sys/petsclib.cxx Outdated Show resolved Hide resolved
@johnomotani johnomotani removed this from the BOUT-4.3 milestone Oct 19, 2019
@johnomotani johnomotani added the backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 label Nov 22, 2019
Copy link
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

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

Thanks for updating this @johnomotani , sorry it's been on the back-burner for so long!

Please run bin/bout-v5-format-upgrader.py to update all the format strings. You might want to save the patch file to undo that change if we also want to back-port this to v4.4.

It would be nice if we can make the unique ID deterministic, I think the options section name is a good bet. Otherwise, checking to see if there's a name key or even passing it as a separate argument might be a good backup plan. Not a blocker, but certainly could make it more ergonomic.

include/bout/petsclib.hxx Outdated Show resolved Hide resolved
include/bout/petsclib.hxx Outdated Show resolved Hide resolved
include/bout/petsclib.hxx Outdated Show resolved Hide resolved
src/sys/petsclib.cxx Outdated Show resolved Hide resolved
src/sys/petsclib.cxx Outdated Show resolved Hide resolved
src/sys/petsclib.cxx Outdated Show resolved Hide resolved
src/invert/laplacexy/laplacexy.cxx Outdated Show resolved Hide resolved
@johnomotani
Copy link
Contributor Author

You might want to save the patch file to undo that change if we also want to back-port this to v4.4.

I don't think I'll have the time/energy to prepare a back-port myself, but I'll make the fmt changes last to make it easier to revert them.

johnomotani and others added 5 commits May 20, 2020 21:35
Co-authored-by: Peter Hill <zed.three@gmail.com>
Might happen that options section is used by more than one solver, but
then the same options will just be (re-)set more than once, which should
not be an error, so options section name is 'unique enough'.
Simplifies calls to PetscLib constructor in the library.
@johnomotani
Copy link
Contributor Author

johnomotani commented May 22, 2020

Note for myself - things in the manual to update before merging:

  • remove/update footnote about prefix \`boutpetsclib#_```
  • note that passing command line options has no effect without prefix - recommend passing e.g. laplace:petsc:ksp_type=gmres instead

Petsc sometimes prints the prefix to identify the solver (e.g. when
using -ksp_monitor). This looks nicer without a trailing underscore.
@johnomotani johnomotani changed the base branch from next to master May 22, 2020 17:54
@johnomotani johnomotani changed the base branch from master to next May 22, 2020 17:54
@johnomotani
Copy link
Contributor Author

The PR tests are failing in the "Coverage (minimal)" test with this compilation error

In file included from bout_test_main.cxx:5:0:
../../include/bout/petsclib.hxx:129:28: error: expected ‘)’ before ‘*’ token
   explicit PetscLib(Options* UNUSED(opt) = nullptr) {}

Anyone have any idea why?

@ZedThree
Copy link
Member

The forward declaration class Options; is inside #if BOUT_HAS_PETSC, so when we compile without PETSc, it's not seen.

The forward-declare is also needed in the #else branch.
@johnomotani
Copy link
Contributor Author

Thanks @ZedThree, that's fixed it! All tests pass now.

@ZedThree
Copy link
Member

Awesome, thanks @johnomotani !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants