From: Vetter R. <vet...@st...> - 2011-02-22 16:43:58
Attachments:
patch.diff
|
Hi guys I propose to add the attached patch to trunk. It adds an integer storing the convergence reason returned by the PETSc nonlinear solver, closely analogous to the existing convergence boolean flag, which is somewhat meager. Without this, the convergence reason is lost right after solving, without allowing the user to find out about it inside their programs (because SNESDestroy(_snes) is called immediately, i.e. before solve() finishes). You may argue that the -snes_converged_reason command line option readily prints the desired information. That's right, but in some cases it is desirable to know INSIDE the C++ program, not just on stdout, at least it is in mine. :-) In the PETSs linear solver (KSP) this information is implemented, just not in the nonlinear one (SNES). What do you think? Roman |
From: Roy S. <roy...@ic...> - 2011-02-22 20:03:10
|
On Tue, 22 Feb 2011, Vetter Roman wrote: > I propose to add the attached patch to trunk. It adds an integer > storing the convergence reason returned by the PETSc nonlinear solver, > closely analogous to the existing convergence boolean flag, which is > somewhat meager. Without this, the convergence reason is lost right > after solving, without allowing the user to find out about it inside > their programs (because SNESDestroy(_snes) is called immediately, i.e. > before solve() finishes). > > You may argue that the -snes_converged_reason command line option > readily prints the desired information. That's right, but in some > cases it is desirable to know INSIDE the C++ program, not just on > stdout, at least it is in mine. :-) In the PETSs linear solver (KSP) > this information is implemented, just not in the nonlinear one (SNES). > > What do you think? I definitely agree with letting the application code access the convergence/divergence reason, but not so much with making it an opaque PETSc-specific int. A new enum_convergence.h would fix all three of those problems at once, if you wanted to go to the trouble. You wouldn't need to bother actually implementing the Trilinos/Laspack codepaths (except to the extent that the base solver class would want to return an "UNKNOWN" or throw a libmesh_not_implemented() by default); I just don't want to make it impossible for someone else to write those implementations later. --- Roy |
From: Roman V. <vet...@st...> - 2011-02-23 10:12:26
|
> I definitely agree with letting the application code access the > convergence/divergence reason, but not so much with making it an > opaque PETSc-specific int. A new enum_convergence.h would fix all > three of those problems at once, if you wanted to go to the trouble. > You wouldn't need to bother actually implementing the Trilinos/Laspack > codepaths (except to the extent that the base solver class would want > to return an "UNKNOWN" or throw a libmesh_not_implemented() by > default); I just don't want to make it impossible for someone else to > write those implementations later. I certainly agree with the enum / making it non-PETSc-specific argument. The reasons why I didn't go there include: - There is a convergence flag in the base class for nonlinear solvers already, and it's set only by the PETSc solver, i.e. for users of other solvers it is misleading (hence I thought, why not introduce yet another misleading variable, haha! No, not really... :-) ) - The PETSc linear solver provides a print_converged_reason() member, which includes the KSPConvergedReason enums, but not all of them! It seems that _ksp is not destroyed right after the solve(), which allows that function to fetch the convergence reason from it. Either that method needs a serious overhaul and the same thing is implemented for the PETSc nonlinear solver as well (including the postponement of _snes destruction), or it is replaced by a new common convergence enum methodology. - The enums are readily defined in petscksp.h and pescsnes.h. Why redefine them? - Different solvers use overlapping enum value ranges. If they were to share a common enum, a remapping would have to be defined. I'm sure that's worth avoiding. Cf. http://www.ib.cnea.gov.ar/~ipc/ptpde/petsc-dir/docs/manualpages/SNES/SNESConvergedReason.html http://www.ib.cnea.gov.ar/~ipc/ptpde/petsc-dir/docs/manualpages/KSP/KSPConvergedReason.html As a libMesh main developer, which approach would you prefer? Roman |
From: Roy S. <roy...@ic...> - 2011-02-28 20:01:42
|
On Wed, 23 Feb 2011, Roman Vetter wrote: > - The PETSc linear solver provides a print_converged_reason() member, > which includes the KSPConvergedReason enums, but not all of them! It > seems that _ksp is not destroyed right after the solve(), which allows > that function to fetch the convergence reason from it. Either that > method needs a serious overhaul That method needs a serious overhaul. I think John added it about 4 years ago, and it works but it's definitely not the ideal design. Using PETSc's convergence reason strings would be a big improvement over making up our own, looking at Jed's suggestion first. > and the same thing is implemented for the PETSc nonlinear solver as > well (including the postponement of _snes destruction), How heavyweight are the SNES and KSP objects? I don't see any non-efficiency reasons against postponing the _snes destruction, and I suspect any efficiency reasons are insignificant enough that "don't automatically destroy the _snes, but give the user an option to do so manually" would be a reasonable alternative. > or it is replaced by a new common convergence enum methodology. I'd prefer to see a common convergence enum methodology regardless; otherwise there's no way to access convergence information from the abstract base classes. > - The enums are readily defined in petscksp.h and pescsnes.h. Why redefine them? Mostly so that codes will still compile when PETSc isn't installed. > - Different solvers use overlapping enum value ranges. If they were to > share a common enum, a remapping would have to be defined. I'm sure > that's worth avoiding. Different solvers use different convergence reporting methods; the only options here are for everybody to write solver-specific code (fine for users to choose, but I don't want libMesh to force that on them) or for libMesh to have some common encapsulation (effectively a remapping) of those values. It does sound like the sort of problem more suited to a class than an enum, though... It would be nice to add more fine-grained convergence/divergence types while still keeping old code forwards compatible via "bool ConvergenceResult::converged()" type methods to query the types' coarse characteristics. --- Roy |
From: Jed B. <je...@59...> - 2011-02-23 10:56:53
|
On Wed, Feb 23, 2011 at 11:12, Roman Vetter <vet...@st...>wrote: > - The PETSc linear solver provides a print_converged_reason() member, > which includes the KSPConvergedReason enums, but not all of them! > Having a switch statement like that is pretty silly. You can get the reason in string form (const char *) with KSPConvergedReasons[reason] The solver has converged if reason>0 and diverged if reason<0. The same applies to SNES, use SNESConvergedReasons[reason] to get the string. |
From: Roman V. <vet...@st...> - 2011-02-23 13:05:41
|
> Having a switch statement like that is pretty silly. You can get the reason > in string form (const char *) with > > KSPConvergedReasons[reason] > > The solver has converged if reason>0 and diverged if reason<0. > > The same applies to SNES, use SNESConvergedReasons[reason] to get the > string. > Exactly. That's kind of the point. In order to be able to call SNESConvergedReasons[reason], the reason enum/int must be stored before _snes gets destroyed, which it isn't currently. Which brings be back to my initial suggestion. Roman |
From: Jed B. <je...@59...> - 2011-02-23 13:21:39
|
On Wed, Feb 23, 2011 at 14:05, Roman Vetter <vet...@st...>wrote: > Exactly. That's kind of the point. In order to be able to call > SNESConvergedReasons[reason], the reason enum/int must be stored > before _snes gets destroyed, which it isn't currently. Which brings be > back to my initial suggestion. > So there are a couple options for exposing reason to the user. 1. Send the raw backend-specific enum to the user. Downside: user needs to write different code for each back end. 2. Always convert to some new libmesh enum. Downside: conversion from backend value to generic value is not injective (several values might be mapped to "unknown", so you have lost information that the user might want. 3. Create a new libmesh enum, but store the backend value. This can always be converted to a generic value when convenient. My comment regarding KSPConvergedReasons is that many PETSc users are familiar with the specific failure modes so it would be entirely reasonable to print it instead of translated strings. It is certainly much better than printing "Unknown/unsupported con(di)vergence". |
From: Roman V. <vet...@st...> - 2011-02-23 14:47:59
|
> So there are a couple options for exposing reason to the user. > > 1. Send the raw backend-specific enum to the user. Downside: user needs to > write different code for each back end. > > 2. Always convert to some new libmesh enum. Downside: conversion from > backend value to generic value is not injective (several values might be > mapped to "unknown", so you have lost information that the user might want. > > 3. Create a new libmesh enum, but store the backend value. This can always > be converted to a generic value when convenient. > > My comment regarding KSPConvergedReasons is that many PETSc users are > familiar with the specific failure modes so it would be entirely reasonable > to print it instead of translated strings. It is certainly much better than > printing "Unknown/unsupported con(di)vergence". > Then I would suggest the following: 1. Replace the contents of PetscLinearSolver::print_converged_reason() by KSPConvergedReason reason; KSPGetConvergedReason(_ksp, &reason); libMesh::out << "Linear solver convergence/divergence reason: " << KSPConvergedReasons[reason] << std::endl; or similar. 2. Remove the this->clear() call from PetscNonlinearSolver::solve(). This prevents the _snes context pointer from being destroyed. In PetscLinearSolver, the _ksp context pointer isn't destroyed either. 3. Introduce PetscNonlinearSolver::print_converged_reason() analogously to the linear solver, using the maintained _snes pointer. This way, maximum backwards compatibility is sustained, full access to the convergence reason is granted via snes() at any time after the solve(), no additional enums need to be implemented, and the linear and nonlinear solvers are synchronized to a more common interface. Or is there a particular reason for destroying the _snes context immediately? Perhaps one should consider clear()ing at the beginning of solve(). Roman |
From: Roy S. <roy...@ic...> - 2011-02-28 20:11:45
|
On Wed, 23 Feb 2011, Roman Vetter wrote: > 1. Replace the contents of PetscLinearSolver::print_converged_reason() by > > KSPConvergedReason reason; > KSPGetConvergedReason(_ksp, &reason); > libMesh::out << "Linear solver convergence/divergence reason: " << > KSPConvergedReasons[reason] << std::endl; > > or similar. This sounds like a good idea, if you want to test it and send us the patch. > 2. Remove the this->clear() call from PetscNonlinearSolver::solve(). > This prevents the _snes context pointer from being destroyed. In > PetscLinearSolver, the _ksp context pointer isn't destroyed either. I'm up for this unless anyone objects. > 3. Introduce PetscNonlinearSolver::print_converged_reason() > analogously to the linear solver, using the maintained _snes pointer. Sounds good. > no additional enums need to be implemented I would have loved getting a patch with those additional enums, but necessity is the mother of invention, there's a long-standing tradition of libMesh developers only inventing what they have a personal necessity for, and there's no reason we should stop now. ;-) --- Roy |
From: Roman V. <vet...@st...> - 2011-03-03 13:54:58
Attachments:
conv_reason.diff
|
>> 1. Replace the contents of PetscLinearSolver::print_converged_reason() by >> >> KSPConvergedReason reason; >> KSPGetConvergedReason(_ksp, &reason); >> libMesh::out << "Linear solver convergence/divergence reason: " << >> KSPConvergedReasons[reason] << std::endl; >> >> or similar. > > This sounds like a good idea, if you want to test it and send us the > patch. > >> 2. Remove the this->clear() call from PetscNonlinearSolver::solve(). >> This prevents the _snes context pointer from being destroyed. In >> PetscLinearSolver, the _ksp context pointer isn't destroyed either. > > I'm up for this unless anyone objects. > >> 3. Introduce PetscNonlinearSolver::print_converged_reason() >> analogously to the linear solver, using the maintained _snes pointer. > > Sounds good. Since there have been no further objections, here's a patch that does all of the above. I have tested both linear (with example 9 for instance) and nonlinear print_converged_reason()s, they're working smoothly in the scope of my testing. Also, no negative side-effects have occurred to me from keeping the _snes context alive. You may perhaps commit the new PetscLinearSolver::print_converged_reason before the rest, if you prefer. Roman |
From: Roy S. <roy...@ic...> - 2011-03-04 03:49:28
|
On Thu, 3 Mar 2011, Roman Vetter wrote: >>> 1. Replace the contents of PetscLinearSolver::print_converged_reason() by >>> >>> KSPConvergedReason reason; >>> KSPGetConvergedReason(_ksp, &reason); >>> libMesh::out << "Linear solver convergence/divergence reason: " << >>> KSPConvergedReasons[reason] << std::endl; >>> >>> or similar. >> >> This sounds like a good idea, if you want to test it and send us the >> patch. >> >>> 2. Remove the this->clear() call from PetscNonlinearSolver::solve(). >>> This prevents the _snes context pointer from being destroyed. In >>> PetscLinearSolver, the _ksp context pointer isn't destroyed either. >> >> I'm up for this unless anyone objects. >> >>> 3. Introduce PetscNonlinearSolver::print_converged_reason() >>> analogously to the linear solver, using the maintained _snes pointer. >> >> Sounds good. > > > Since there have been no further objections, here's a patch that does > all of the above. I have tested both linear (with example 9 for > instance) and nonlinear print_converged_reason()s, they're working > smoothly in the scope of my testing. Also, no negative side-effects > have occurred to me from keeping the _snes context alive. > > You may perhaps commit the new > PetscLinearSolver::print_converged_reason before the rest, if you > prefer. This looks good to me, and is now committed. Hopefully John etc. will fix it if there are any PETSc 2 compatibility issues. ;-) --- Roy |
From: Vijay S. M. <vi...@gm...> - 2011-03-04 04:01:33
|
Roy, I think it would be advisable that all libMesh users update themselves to atleast petsc/slepc 3.0-p0. There are so many fixes/changes from 2.3.3.->3.0 that it feels like they dont utilize the full potential of the solvers. Of course, the petsc_macro preprocessor routines would shield the user anyhow but my point is, should we actually encourage this ? It is harder to maintain eventually as new APIs are introduced and the gain is quite minimal. I am sure the petsc developers will probably agree with this too since they constantly remind the users to upgrade to the newer version n the mailing lists. Just my 2 cents. Vijay On Thu, Mar 3, 2011 at 9:49 PM, Roy Stogner <roy...@ic...> wrote: > > > On Thu, 3 Mar 2011, Roman Vetter wrote: > >>>> 1. Replace the contents of PetscLinearSolver::print_converged_reason() by >>>> >>>> KSPConvergedReason reason; >>>> KSPGetConvergedReason(_ksp, &reason); >>>> libMesh::out << "Linear solver convergence/divergence reason: " << >>>> KSPConvergedReasons[reason] << std::endl; >>>> >>>> or similar. >>> >>> This sounds like a good idea, if you want to test it and send us the >>> patch. >>> >>>> 2. Remove the this->clear() call from PetscNonlinearSolver::solve(). >>>> This prevents the _snes context pointer from being destroyed. In >>>> PetscLinearSolver, the _ksp context pointer isn't destroyed either. >>> >>> I'm up for this unless anyone objects. >>> >>>> 3. Introduce PetscNonlinearSolver::print_converged_reason() >>>> analogously to the linear solver, using the maintained _snes pointer. >>> >>> Sounds good. >> >> >> Since there have been no further objections, here's a patch that does >> all of the above. I have tested both linear (with example 9 for >> instance) and nonlinear print_converged_reason()s, they're working >> smoothly in the scope of my testing. Also, no negative side-effects >> have occurred to me from keeping the _snes context alive. >> >> You may perhaps commit the new >> PetscLinearSolver::print_converged_reason before the rest, if you >> prefer. > > This looks good to me, and is now committed. Hopefully John etc. will > fix it if there are any PETSc 2 compatibility issues. ;-) > --- > Roy > > ------------------------------------------------------------------------------ > What You Don't Know About Data Connectivity CAN Hurt You > This paper provides an overview of data connectivity, details > its effect on application quality, and explores various alternative > solutions. http://p.sf.net/sfu/progress-d2d > _______________________________________________ > Libmesh-devel mailing list > Lib...@li... > https://lists.sourceforge.net/lists/listinfo/libmesh-devel > |
From: John P. <pet...@cf...> - 2011-03-04 18:08:31
|
On Thu, Mar 3, 2011 at 9:01 PM, Vijay S. Mahadevan <vi...@gm...> wrote: > Roy, > > I think it would be advisable that all libMesh users update themselves > to atleast petsc/slepc 3.0-p0. There are so many fixes/changes from > 2.3.3.->3.0 that it feels like they dont utilize the full potential of > the solvers. Of course, the petsc_macro preprocessor routines would > shield the user anyhow but my point is, should we actually encourage > this ? I don't think it's a matter of "encouraging" users to continue on 2.3.3 or even to pick up 2.3.3, but rather of maintaining compatibility with 2.3.3 as a libmesh "feature". The relatively minimal (2.3.3 isn't changing any more after all!) amount of work required to maintain this feature is worthwhile and especially beneficial to large install bases where it will still take some time (and system administration) to switch to the 3.x series, even if that switch-over is currently in progress. -- John |