Jed : > Hong, as far as I can tell, none of these changes should be merged to > petsc-dev right now. If you go through with the merge, use hg diff -r > SHA1_OF_UPSTREAM_PETSC_DEV to make sure that none of this 3.3 stuff gets > merged. Otherwise, one of us can do the merge.
I quit my merge and wait for someone else does it :-( Hong > > > On Tue, Aug 21, 2012 at 11:08 AM, Hong Zhang <hzhang at mcs.anl.gov> wrote: > >> Dmitry : >> I pushed a bugfix of mumps interface to 3.3. When merging it to >> petsc-dev, I get >> >> merging src/ksp/pc/impls/fieldsplit/fieldsplit.c failed! >> >> Hong >> >> >>> >>> On Mon, Aug 20, 2012 at 8:36 AM, Jed Brown <jedbrown at mcs.anl.gov> wrote: >>> >>>> On Mon, Aug 20, 2012 at 4:02 AM, Dmitry Karpeev <karpeev at >>>> mcs.anl.gov>wrote: >>>> >>>>> >>>>> >>>>> On Sun, Aug 19, 2012 at 1:17 PM, Jed Brown <jedbrown at mcs.anl.gov>wrote: >>>>> >>>>>> On Sun, Aug 19, 2012 at 12:48 PM, Dmitry Karpeev <karpeev at mcs.anl.gov >>>>>> > wrote: >>>>>> >>>>>>> The main reason for this being a patch in 3.3 is that recursive >>>>>>> FieldSplit is broken there (which is why I couldn't enable it in >>>>>>> libMesh/Moose correctly). >>>>>> >>>>>> >>>>>> The current interface doesn't explicitly support all the ways to get >>>>>> information into the split, but several people have worked around the >>>>>> limitations to get the necessary information in there. Having you and >>>>>> Matt >>>>>> changing things to rely on mutually incompatible side-effects does not >>>>>> help. >>>>>> >>>>> I don't think the present fix is about the API or working through side >>>>> effects (nullspace stuff excluded and partially backed out here: >>>>> http://petsc.cs.iit.edu/petsc/releases/petsc-3.3/rev/1723d4624521) >>>>> >>>> >>>> What do you call this external DM futzing? >>>> >>> It's no different than holding onto the ISs set in >>> PCFieldSplitSetDefaults() or directly via PCFieldSplitSetIS() until they >>> can be used in PCSetUp_FieldSplit(). I don't see how this can be avoided >>> here or in the future, unless we do something to fundamentally change the >>> set up process. >>> >>>> >>>> >>>>> The current interface assumes that the A00 solver and the Schur >>>>> inner solver are to be set up identically >>>>> >>>> >>>> If they are being set up identically, they should literally be the same >>>> object. This is often the biggest setup cost in the whole problem (e.g. >>>> AMG). >>>> >>> I agree, but that's available only in petsc-dev, not in petsc-3.3. The >>> duplicate setup has been occurring all along, now it is actually consistent >>> with the DM being forwarded to the inner solver. >>> >>>> >>>> >>>>> (something we relaxed in petsc-dev). In order to use fieldsplit on >>>>> these solvers the DM for the corresponding split must be forwarded to both >>>>> of these A00 solvers. It was being set only on the "outer" A00 solver. >>>>> Partly this is an artifact of our "split" setup process: some of it >>>>> occurs >>>>> in PCFieldSplitSetDefaults(), some later in PCSetUp_FieldSplit(). Some >>>>> objects obtained in PCFieldSplitSetDefaults() need to be cached until >>>>> PCSetUp_FieldSplit(), and the splits' DMs are among those. I now put in >>>>> proper reference counting for them here: >>>>> http://petsc.cs.iit.edu/petsc/releases/petsc-3.3/rev/188af9799779 >>>>> >>>>> >>>>>> Maybe we can reach some agreement on the proper way to do things. >>>>>> >>>>> That's fine with me. The reason I wanted to fix this particular >>>>> problem now is that some dependent packages (e.g., Moose) only rely on >>>>> release versions of petsc and would not be able to use this functionality >>>>> properly for quite some time. >>>>> >>>> >>>> Why do packages do this? Because petsc-dev is sometimes unstable. >>>> >>>> Now what happens when code that is pushed to the release changes >>>> behavior and uses uninitialized memory? The release becomes unstable. >>>> >>>> Even worse, our nightly testing is targeted at petsc-dev because the >>>> expectation is that only trivial and well-tested bug-fix patches are pushed >>>> to release. If we are changing that model by introducing significant >>>> changes, we MUST have nightly tests for the release, we have to actually >>>> look at the results, and I would be strongly in favor of switching from >>>> 3.3p3 numbering to 3.3.1. >>>> >>>> In all cases, these subminor or patch releases MUST be binary and >>>> source-level backward compatible. >>>> >>>> >>>>> Let's fix that "down the line". Setting a DM should never force it to >>>>>> be used or cause an error due to unsupported operation in a case where >>>>>> not >>>>>> having a DM is also acceptable. >>>>>> >>>>> Currently the inner and outer A00 solvers are essentially identified >>>>> (duplicated), so they should be set up identically, including the DM. >>>>> Especially when that DM defines a recursive split. This is in part >>>>> because there is no way to configure those two differently >>>>> programmatically >>>>> or from the command line. So if the outer A00 is using >>>>> -fieldsplit_0_pc_type_fieldsplit, the inner A00 will as well. >>>>> However, the DM on the inner A00 will be rather different (or absent) >>>>> and produce splits incompatible with the rest of the options (e.g., a >>>>> single default split, while -fieldsplit_0_pc_fieldsplit_type schur). >>>>> >>>>> Incidentally, this raises this question down the road: in petsc-dev >>>>> the inner and outer A00 can be configure separately using different >>>>> prefixes, but how should the inner A00 DM be configured? >>>>> >>>> >>>> The KSPs should only be different if the solver is different. Having >>>> them separate, but using the same DM is one reason I was not wild about >>>> having physics in the DM, but I don't see a good way to plumb in the extra >>>> information. In any case, we need to come up with a solution before pushing >>>> code. >>>> >>>> >>>>> Okay, I'll back out that part of the second patch, since A11's >>>>>>> nullspace is assumed to be meant for S. I'm not sure what's incorrect >>>>>>> about the comment, though: if a vector is in the A11's kernel, but not >>>>>>> in >>>>>>> S's, I call it a "false positive". This terminology may be wanting, but >>>>>>> what's incorrect about it? >>>>>>> >>>>>> >>>>>> "force an inconsistent rhs" >>>>>> >>>>> The KSP solve will fail only if the search space X is contracted to >>>>> the point where the rhs b is not in the range AX, hence, >>>>> inconsistent. >>>>> >>>> >>>> Okay, I just would have said that it changes the operator. Also, the >>>> Krylov methods are not necessarily correct if the matrix does not have the >>>> stated null space (since it's expected that the matrix has the null space >>>> and only the preconditioner needs to be filtered). >>>> >>>> >>>>> I backed out this hunk, however. >>>>> >>>>>> >>>>>> S may well be nonsingular. >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> I think that's confusing and we need a specific API for it, but if >>>>>>>> people are using it that way, we shouldn't change it in 3.3. >>>>>>>> >>>>>>>> I'd prefer to backout the first patch, split it into separate >>>>>>>> pieces, and review each before pushing. I think it's a potentially big >>>>>>>> behavior change for 3.3. >>>>>>>> >>>>>>> Which pieces of the first patch do you think are too big for 3.3? >>>>>>> The splits' and Schur KSPs have to be set up (maybe not all in every >>>>>>> situation) in order for recursive splitting to work. Should we declare >>>>>>> that capability unavailable for 3.3? I can eliminate the unneeded >>>>>>> KSPSetUp() calls. >>>>>>> >>>>>> >>>>>> 1. I hate those hacky dm reference-non-references. >>>>>> >>>>>> 2. I don't want unused KSPs to be set up. (We should eventually fix >>>>>> the model so they don't exist...) >>>>>> >>>>>> 3. Two of the three IncrementTabLevels that you introduced are not >>>>>> necessary because the KSP's tab level was incremented before getting the >>>>>> PC. >>>>>> >>>>> Where? >>>>> >>>> >>>> ierr = >>>> KSPCreate(((PetscObject)pc)->comm,&jac->kspschur);CHKERRQ(ierr); >>>> ierr = >>>> PetscLogObjectParent((PetscObject)pc,(PetscObject)jac->kspschur);CHKERRQ(ierr); >>>> ierr = >>>> PetscObjectIncrementTabLevel((PetscObject)jac->kspschur,(PetscObject)pc,1);CHKERRQ(ierr); >>>> + { >>>> + PC pcschur; >>>> + ierr = KSPGetPC(jac->kspschur, &pcschur); >>>> CHKERRQ(ierr); >>>> + ierr = >>>> PetscObjectIncrementTabLevel((PetscObject)pcschur,(PetscObject)pc,1);CHKERRQ(ierr); >>>> + } >>>> >>>> >>>> ierr = >>>> KSPCreate(((PetscObject)pc)->comm,&ilink->ksp);CHKERRQ(ierr); >>>> ierr = >>>> PetscObjectIncrementTabLevel((PetscObject)ilink->ksp,(PetscObject)pc,1);CHKERRQ(ierr); >>>> + { >>>> + PC ilinkpc; >>>> + ierr = KSPGetPC(ilink->ksp, &ilinkpc); CHKERRQ(ierr); >>>> + ierr = >>>> PetscObjectIncrementTabLevel((PetscObject)ilinkpc,(PetscObject)pc,1);CHKERRQ(ierr); >>>> + } >>>> >>>> >>>> This is the pattern used everywhere else in PETSc so that sub-pcs >>>> always have the correct tab level. >>>> >>> >>> >> > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20120821/655c3718/attachment-0001.html>
