[petsc-dev] use of deprecated DMSetFunction() approach

[Barry Smith]

On Nov 25, 2012, at 4:16 AM, Jed Brown <jedbrown at mcs.anl.gov> wrote:

> On Sun, Nov 25, 2012 at 5:20 AM, Barry Smith <bsmith at mcs.anl.gov> wrote:
> 
>    Matt,
> 
>      The use of DMSetFunction/Jacobian was deprecated many months ago when KSPDM, SNESDM, and TSDM were introduced but you seem to be merrily using it to build a complete DMSetLocalFunction() infrastructure?   I already gave Jed and Peter a serious tongue lashing for "providing backward compatible support for DMSetFunction()" and not completely stripping it out when they wrote the replacement, as the PETSc style guide requires they should have done.
> 
> 
>    Everyone,
> 
>    But now what are we going to do? We need to support
> 
> 1) the usual "global" SNES/TS/KSP Set Function/Jacobian
> 
> 2) special DM specific function/Jacobian evaluations such as
> 
>      a)  finite elment style SNES/TS/KSP Set Local (ghosted) Function/Jacobin
> 
>       b) DA oriented set local function/Jacobian
> 
> 
> The KSPDM/SNESDM/TSDM model seems ok for managing 1)  but how do we plan to manage all the 2)?  
> 
> DMDASNESSetFunctionLocal() and similar. To use those routines, you must "know about DM", thus there is no hardship in setting your callbacks on the DM.
> 
> Note that SNESSetFunction() trivially redirects into DMSNESSetFunction() so it is doing nothing more than hiding DM from users that are not interested in using it.
>  
> The comments in the code
> 
>   /* This context/destroy pair allows implementation-specific routines such as DMDA local functions. */
>   PetscErrorCode (*destroy)(KSPDM);
>   void *data;
> 
> A duplicate routine should be here, but nobody has written DMDAKSPSetComputeOperators() so it wasn't being used.
> 
> https://bitbucket.org/petsc/petsc-dev/changeset/026daea85c5c24fc64f04357af2f13afc6b23697
>  
> 
>   /* This context/destroy pair allows implementation-specific routines such as DMDA local functions. */
>   PetscErrorCode (*destroy)(SNESDM);
>   PetscErrorCode (*duplicate)(SNESDM,DM);
>   void *data;
> 
>   /* This context/destroy pair allows implementation-specific routines such as DMDA local functions. */
>   PetscErrorCode (*destroy)(TSDM);
>   void *data;
> 
> Duplicate was needed here.
> 
> https://bitbucket.org/petsc/petsc-dev/changeset/d5619197e506fdb0622ba24b0e3c26ddbee596aa
>  
> 
> seem to indicate someone has thought about his but how the f it is planned to be done is unclear (and why SNES requires a duplicate but KSP and TS do not?).  In particular what is the user interface would it be
> 
> SNESDMDASetLocalFunction/Jacobian()?  or DMDASNESSetLocalFunction/Jacobian()?
> 
> DMDASNESSetFunctionLocal(), it has been there since March.

   Totally awesome!  And can I ask why someone who made the obvious effort to write all this cool better new code didn't spend ten minutes to change the use of DMDASetFunctionLocal() in the examples to the new format  and strip out the old code that did the outdated wrong way?   Support for backward capability is the work of the devil, just don't do it!

   Ok I will try to update the examples shortly and strip out that old code that should have been removed in March.

   Barry



>  
> 
> I am also bothered by the more fundamental question of what is the expected user interface when there exists both
> 
> SNESSetFunction()   and DMSNESSetFunction()?
> 
> Are users suppose to either of them or just SNESSetFunction()? If just SNESSetFunction() then why is the level of DMSNESSetFunction() just advanced and not developer.   Having both of these is a major recipe for complete confusion for both users and developers.
> 
> My expectation is that 90% of users of DM will be able to use the impl-specific local routines like DMDASNESSetFunctionLocal(). For the few that need something more general (e.g., multiple communication phases in residual evaluation), they could use either. DMSNESSetFunction() is slightly more powerful than SNESSetFunction() because it can be set independent of the SNES, but I think that is rarely important.
> 
> SNESSetFunction() is purely cosmetic, allowing people to use SNES without needing to know about the DM concept. We can delete it if "more than one way to do it" is worse that "yet another object to interact with", but I'm doubtful.
>  
> 
> Also all the half-assed legacy support crap that has gotten in there makes the code incredibly fragile and is harder to get rid of then it should be.
> 
> We need to pick a single consistent extensible model now and change everything to match that model, the current code makes us look like a bunch of Trilinos developers.
> 
>    Barry