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>