<div class="gmail_quote">On Tue, Aug 21, 2012 at 2:53 AM, Dmitry Karpeev <span dir="ltr"><<a href="mailto:karpeev@mcs.anl.gov" target="_blank">karpeev@mcs.anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><br><div class="gmail_quote"><div class="im">On Mon, Aug 20, 2012 at 8:36 AM, Jed Brown <span dir="ltr"><<a href="mailto:jedbrown@mcs.anl.gov" target="_blank">jedbrown@mcs.anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>On Mon, Aug 20, 2012 at 4:02 AM, Dmitry Karpeev <span dir="ltr"><<a href="mailto:karpeev@mcs.anl.gov" target="_blank">karpeev@mcs.anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><br><div class="gmail_quote"><div>On Sun, Aug 19, 2012 at 1:17 PM, Jed Brown <span dir="ltr"><<a href="mailto:jedbrown@mcs.anl.gov" target="_blank">jedbrown@mcs.anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>On Sun, Aug 19, 2012 at 12:48 PM, Dmitry Karpeev <span dir="ltr"><<a href="mailto:karpeev@mcs.anl.gov" target="_blank">karpeev@mcs.anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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).</blockquote></div><div><br>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. </div>
</div></blockquote></div><div>I don't think the present fix is about the API or working through side effects (nullspace stuff excluded and partially backed out here: <a href="http://petsc.cs.iit.edu/petsc/releases/petsc-3.3/rev/1723d4624521" target="_blank">http://petsc.cs.iit.edu/petsc/releases/petsc-3.3/rev/1723d4624521</a>)</div>
</div></blockquote><div><br></div></div><div>What do you call this external DM futzing?</div></div></blockquote></div><div>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.</div>
</div></blockquote><div><br></div><div>As I said earlier, I think we should use KSPSetDM/KSPGetDM. A DMShell should not be a problem because it just won't implement some routines. At least in 3.3, there are probably cases where ksp->dm is misused, but we should fix those places instead of cluttering fieldsplit.</div>
<div><br></div><div>In your revised patch, why this extra conditional?</div><div><div><br>+ ilink->dm = dms[i];</div></div><div><div>+ if(ilink->dm) {</div><div>+ ierr = PetscObjectReference((PetscObject)ilink->dm);CHKERRQ(ierr);</div>
<div>+ }</div></div><div><br>Is it the case that DMCreateFieldDecomposition() can return an array containing some DMs and some NULL? If so, I think we should change it so that a DM is always returned (even if it's just a DMShell).</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote">
<div>The current interface assumes that the A00 solver and the Schur inner solver are to be set up identically</div></div></blockquote><div><br></div></div><div>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).</div>
</div></blockquote></div><div>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. </div>
</div></blockquote><div><br></div><div>With Schur, we don't even use the link->ksp. Look at PCApply_FieldSplit_Schur(). Why are we setting this thing up? Where in PCSetUp_FieldSplit() is it really being set up?<br>
<br>I also don't like part of <a href="http://petsc.cs.iit.edu/petsc/releases/petsc-3.3/rev/3c0d43fb5911">http://petsc.cs.iit.edu/petsc/releases/petsc-3.3/rev/3c0d43fb5911</a><br></div><div><br><div>+ /* </div><div>
+ Set from options only the A00 split. The other split's solver won't be used with Schur. </div><div>+ Should it be destroyed? Should KSPCreate() be moved here from PCFieldSplitSetIS() and invoked </div>
<div>+ only when necessary? </div><div>+ */</div><div>+ ierr = KSPSetFromOptions(jac->head->ksp);CHKERRQ(ierr);</div><div>+</div><div> /* need to handle case when one is resetting up the preconditioner */</div>
<div> if (jac->schur) {</div></div><div><br></div><div>This call is being done every time through PCSetUp_FieldSplit(), which it shouldn't be. It's also being called on a KSP that is not being used for anything (e.g. look at the "if (jac->type == PC_COMPOSITE_SCHUR) {" part of PCSetUp_FieldSplit().<br>
<br><br>Now, checking to see what has happened in this flurry of patches and partial reversions:<br><br>$ hg diff -r dbc8d9af8577 src/ksp/pc/impls/ # revision is before all of your changes<br><br>I would like to get rid of these superfluous hunks that we just talked about</div>
<div><br><div> ierr = KSPCreate(((PetscObject)pc)->comm,&jac->kspschur);CHKERRQ(ierr);</div><div> ierr = PetscLogObjectParent((PetscObject)pc,(PetscObject)jac->kspschur);CHKERRQ(ierr);</div><div>
ierr = PetscObjectIncrementTabLevel((PetscObject)jac->kspschur,(PetscObject)pc,1);CHKERRQ(ierr);</div><div>+ {</div><div>+ PC pcschur;</div><div>+ ierr = KSPGetPC(jac->kspschur, &pcschur); CHKERRQ(ierr);</div>
<div>+ ierr = PetscObjectIncrementTabLevel((PetscObject)pcschur,(PetscObject)pc,1);CHKERRQ(ierr);</div><div>+ }</div></div><div><br><div>@@ -1108,6 +1152,11 @@</div><div> ilink->next = PETSC_NULL;</div>
<div> ierr = KSPCreate(((PetscObject)pc)->comm,&ilink->ksp);CHKERRQ(ierr);</div><div> ierr = PetscObjectIncrementTabLevel((PetscObject)ilink->ksp,(PetscObject)pc,1);CHKERRQ(ierr);</div>
<div>+ { </div><div>+ PC ilinkpc;</div><div>+ ierr = KSPGetPC(ilink->ksp, &ilinkpc); CHKERRQ(ierr);</div><div>+ ierr = PetscObjectIncrementTabLevel((PetscObject)ilinkpc,(PetscObject)pc,1);CHKERRQ(ierr);</div>
<div>+ }</div></div><div><br><br></div><div><br></div><div>I thought we agreed in this thread that we were (for now) going with Matt's bastardized model of attaching the Schur null space to A11. Doesn't that mean that this hunk should also be reverted (and have a comment explaining this indirect effect)?<br>
<br><div>@@ -568,31 +586,54 @@</div><div> /* Use mat[0] (diagonal block of the real matrix) preconditioned by pmat[0] */</div><div> ierr = MatCreateSchurComplement(jac->mat[0],jac->pmat[0],jac->B,jac->C,jac->mat[1],&jac->schur);CHKERRQ(ierr);</div>
<div> ierr = MatGetNullSpace(jac->pmat[1], &sp);CHKERRQ(ierr);</div><div>- if (sp) {ierr = MatSetNullSpace(jac->schur, sp);CHKERRQ(ierr);}</div><div>- /* set tabbing and options prefix of KSP inside the MatSchur */</div>
<div>+ /* </div><div>+ Do we really want to attach the A11-block's nullspace to S? Note that this may generate</div><div>+ "false positives", when the A11's nullspace isn't S's: Stokes or A = [1, 1; 1, 0].</div>
<div>+ Using a false nullspace may prevent KSP(S) from converging, since it might force an inconsistent rhs. </div><div>+ */</div><div>+ /* if (sp) {ierr = MatSetNullSpace(jac->schur, sp);CHKERRQ(ierr);} */</div>
<div>+ /* set tabbing, options prefix and DM of KSP inside the MatSchurComplement */</div></div><div><br></div><div><br></div><div><br></div><div><br></div><div>Also, I like to use hg backout to revert changesets because then we don't end up with trivial formatting changes like this in the diff (new violations of Barry's formatting guidelines, no less)<br>
<br><div>@@ -604,15 +645,17 @@</div><div> ierr = PetscObjectQuery((PetscObject)pc->pmat,lscname,(PetscObject*)&LSC_L);CHKERRQ(ierr);</div><div> if (!LSC_L) {ierr = PetscObjectQuery((PetscObject)pc->mat,lscname,(PetscObject*)&LSC_L);CHKERRQ(ierr);}</div>
<div> if (LSC_L) {ierr = PetscObjectCompose((PetscObject)jac->schur,"LSC_Lp",(PetscObject)LSC_L);CHKERRQ(ierr);}</div><div>- } else {</div><div>- /* set up the individual PCs */</div><div>+ } </div><div>
+ else {</div><div>+ /* set up the individual splits' PCs */<br><br><div>- if (!jac->suboptionsset) {ierr = KSPSetFromOptions(ilink->ksp);CHKERRQ(ierr);}</div></div></div><div><div>+ if (!jac->suboptionsset) {</div>
<div>+ ierr = KSPSetFromOptions(ilink->ksp);CHKERRQ(ierr);</div><div>+ }</div></div><div><br></div><div><br><div> }</div><div>- ierr = PetscSNPrintf(schurprefix,sizeof schurprefix,"%sfieldsplit_%s_",((PetscObject)pc)->prefix?((PetscObject)pc)->prefix:"",ilink->splitname);CHKERRQ(ierr);</div>
<div>- ierr = KSPSetOptionsPrefix(jac->kspschur,schurprefix);CHKERRQ(ierr);</div><div> /* really want setfromoptions called in PCSetFromOptions_FieldSplit(), but it is not ready yet */</div><div> /* need to call this every time, since the jac->kspschur is freshly created, otherwise its options never get set */</div>
<div>- ierr = KSPSetFromOptions(jac->kspschur);CHKERRQ(ierr);</div><div>+ ierr = PetscSNPrintf(schurprefix,sizeof schurprefix,"%sfieldsplit_%s_",((PetscObject)pc)->prefix?((PetscObject)pc)->prefix:"",ilink->splitname);CHKERRQ(ierr);</div>
<div>+ ierr = KSPSetOptionsPrefix(jac->kspschur,schurprefix);CHKERRQ(ierr);</div><div>+ ierr = KSPSetFromOptions(jac->kspschur); CHKERRQ(ierr);</div></div><div><br></div></div>