I'll be in transit tomorrow, but I'll look at this on Thursday. Unfortunately, right now fieldsplit in petsc-dev is sufficiently different from petsc-3.3 (it has been for a while, really) for an automerge.
Dmitry. On Tue, Aug 21, 2012 at 11:29 AM, Hong Zhang <hzhang at mcs.anl.gov> wrote: > 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/1cd224d0/attachment.html>
