<div dir="ltr">Matt,<div>This bug is fixed and merged to master. Please give it a try and let me know if there is a problem.</div><div>Hong</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 23, 2017 at 7:04 PM, Zhang, Hong <span dir="ltr"><<a href="mailto:hzhang@mcs.anl.gov" target="_blank">hzhang@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">OK, I removed flag MAT_SUBMAT_SINGLEIS<br>
<a href="https://bitbucket.org/petsc/petsc/commits/65b5f10fe03da0cc383d7dd0f31fadf6a7ab9abe" rel="noreferrer" target="_blank">https://bitbucket.org/petsc/<wbr>petsc/commits/<wbr>65b5f10fe03da0cc383d7dd0f31fad<wbr>f6a7ab9abe</a><br>
<span class=""><br>
Hong<br>
______________________________<wbr>__________<br>
From: Barry Smith [<a href="mailto:bsmith@mcs.anl.gov">bsmith@mcs.anl.gov</a>]<br>
</span>Sent: Monday, January 23, 2017 2:29 PM<br>
<span class="">To: Zhang, Hong<br>
Cc: Matthew Knepley; PETSc<br>
Subject: Re: [petsc-dev] Bug introduced in MatGetSubmatrices()<br>
<br>
</span><span class="">> On Jan 23, 2017, at 2:23 PM, Zhang, Hong <<a href="mailto:hzhang@mcs.anl.gov">hzhang@mcs.anl.gov</a>> wrote:<br>
><br>
> We actually check if ismax=1 for all process in PCSetUp_ASM():<br>
> if (outwork.max == 1 && outwork.sum == size) {<br>
> /* osm->n_local_true = 1 on all processes, set this option may enable use of optimized MatGetSubMatrices() implementation */<br>
> ierr = MatSetOption(pc->pmat,MAT_<wbr>SUBMAT_SINGLEIS,PETSC_TRUE);<wbr>CHKERRQ(ierr);<br>
> }<br>
> which is free of communication, then set the flag to avoid any MPI_Allreduce call in<br>
> MatGetSubMatrices(). Without this flag, it calls general impl of MatGetSubMatrices() which calls reduce once.<br>
<br>
</span> Oh, the problem is that pc->pmat now has this flag and if someone calls any other MatGetSubMatrices() on this matrix the flag is set and thus trouble ensues. So the flag can't be stashed in the matrix.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> Hong<br>
><br>
> ______________________________<wbr>__________<br>
> From: Barry Smith [<a href="mailto:bsmith@mcs.anl.gov">bsmith@mcs.anl.gov</a>]<br>
> Sent: Monday, January 23, 2017 11:34 AM<br>
> To: Zhang, Hong<br>
> Cc: Matthew Knepley; PETSc<br>
> Subject: Re: [petsc-dev] Bug introduced in MatGetSubmatrices()<br>
><br>
>> On Jan 23, 2017, at 10:51 AM, Zhang, Hong <<a href="mailto:hzhang@mcs.anl.gov">hzhang@mcs.anl.gov</a>> wrote:<br>
>><br>
>><br>
>><br>
>> What if a MPI process has no submatrices, how does it save the flag?<br>
>><br>
>> case of 'SingleIS' is for ismax=1 in all processes. For PCASM, all process has ismax=1, isn't it?<br>
><br>
> No, definitely not always 1.<br>
><br>
>><br>
>> Hong<br>
>><br>
>><br>
>>> On Jan 22, 2017, at 8:58 PM, Zhang, Hong <<a href="mailto:hzhang@mcs.anl.gov">hzhang@mcs.anl.gov</a>> wrote:<br>
>>><br>
>>> The 'saved' context is in submatrices, not original matrix. I made following fix:<br>
>>><br>
>>> if (C->submat_singleis) { /* flag is set in PCSetUp_ASM() to skip MPIU_Allreduce() */<br>
>>> ierr = MatGetSubMatrices_MPIAIJ_<wbr>SingleIS(C,ismax,isrow,iscol,<wbr>scall,submat);CHKERRQ(ierr);<br>
>>> + C->submat_singleis = PETSC_FALSE; /* resume its default value in case C will be used for non-singlis */<br>
>>> PetscFunctionReturn(0);<br>
>>> }<br>
>>> ...<br>
>>> + } else { /* MAT_REUSE_MATRIX */<br>
>>> subc = (Mat_SeqAIJ*)((*submat)[0]-><wbr>data);<br>
>>> smat = subc->submatis1;<br>
>>> if (!smat) {<br>
>>> /* smat is not generated by MatGetSubMatrix_MPIAIJ_All(...<wbr>,MAT_INITIAL_MATRIX,...) */<br>
>>> wantallmatrix = PETSC_TRUE;<br>
>>> + } else if (smat->singleis) {<br>
>>> + ierr = MatGetSubMatrices_MPIAIJ_<wbr>SingleIS(C,ismax,isrow,iscol,<wbr>scall,submat);CHKERRQ(ierr);<br>
>>> + PetscFunctionReturn(0);<br>
>>> } else {<br>
>>><br>
>>> ...<br>
>>> ie., after first call of MatGetSubMatrices_MPIAIJ_<wbr>SingleIS, this flag is set in saved context of submatrices<br>
>>> and is reset as 'false' to the original matrix C.<br>
>>><br>
>>> Do you agree with this fix? Is so, I'll push it in a branch for nightly test.<br>
>>><br>
>>> Hong<br>
>>><br>
>>> ______________________________<wbr>__________<br>
>>> From: Barry Smith [<a href="mailto:bsmith@mcs.anl.gov">bsmith@mcs.anl.gov</a>]<br>
>>> Sent: Friday, January 20, 2017 8:19 PM<br>
>>> To: Matthew Knepley<br>
>>> Cc: Zhang, Hong; PETSc<br>
>>> Subject: Re: [petsc-dev] Bug introduced in MatGetSubmatrices()<br>
>>><br>
>>> Ok, so the problem comes from setting the flag in in the original matrix that other people have access to and can be used for other purposes by anyone,.<br>
>>><br>
>>> So it would be better to instead put the flag into the generated sub matrix which no one else has access to. This should be do-able and is the right way to do it anyways.<br>
>>><br>
>>> Hong, can you try putting all the "saved" context into the generated submatrices instead of the original matrix? If that is not possible then the context saved with the original matrix should have a reference to the sub matrix and this context should only be reused if the sub matrix passed in matches the sub matrix referenced in the context (this is a less desirable solution).<br>
>>><br>
>>> Barry<br>
>>><br>
>>>> On Jan 20, 2017, at 8:06 PM, Matthew Knepley <<a href="mailto:knepley@gmail.com">knepley@gmail.com</a>> wrote:<br>
>>>><br>
>>>> On Fri, Jan 20, 2017 at 7:39 PM, Barry Smith <<a href="mailto:bsmith@mcs.anl.gov">bsmith@mcs.anl.gov</a>> wrote:<br>
>>>><br>
>>>>> On Jan 20, 2017, at 7:01 PM, Matthew Knepley <<a href="mailto:knepley@gmail.com">knepley@gmail.com</a>> wrote:<br>
>>>>><br>
>>>>> On Fri, Jan 20, 2017 at 3:08 PM, Barry Smith <<a href="mailto:bsmith@mcs.anl.gov">bsmith@mcs.anl.gov</a>> wrote:<br>
>>>>><br>
>>>>>> On Jan 20, 2017, at 3:01 PM, Zhang, Hong <<a href="mailto:hzhang@mcs.anl.gov">hzhang@mcs.anl.gov</a>> wrote:<br>
>>>>>><br>
>>>>>> We set C->submat_singleis = false as default.<br>
>>>>>> User turns it on when he knows a single IS will be used for MatGetSubMatrices(), e.g., when asm is used.<br>
>>>>>> User should turn the flag off after it is being used.<br>
>>>>><br>
>>>>> Right, this is why I am confused why this is messing up Mat since presumably he is not setting it?<br>
>>>>><br>
>>>>> ASM is turning this flag on, and it is corrupting my UNRELATED call to MatGetSubMatrices().<br>
>>>><br>
>>>> Yes, but you need to tell us exactly how. The flag is set into C->submat_singleis which is associated with the C from ASM, so why is it affecting your UNRELATED call to MatGetSubMatrices(). It is not a global variable so shouldn't.<br>
>>>><br>
>>>> It sets the flag on the primary matrix. I will go step by step<br>
>>>><br>
>>>> 1) I have a linear system with matrix A<br>
>>>><br>
>>>> 2) To form a preconditioner for FieldSplit, I extract some diagonal blocks from A using MatGetSubMatrices()<br>
>>>><br>
>>>> 3) I also use ASM on the another block of FS<br>
>>>><br>
>>>> 4) The second Newton iterate, I try to call MatGetSubMatrices(), but ASM has set a flag on the matrix that causes this to fail.<br>
>>>><br>
>>>> Explain or send a code that reproduces the problem.<br>
>>>><br>
>>>> You can easily reproduce this by installing PyLith (a widely used package) and running an automatic test. However, since this<br>
>>>> is so simple, I am not sure this is necessary.<br>
>>>><br>
>>>> Matt<br>
>>>><br>
>>>> Barry<br>
>>>><br>
>>>>><br>
>>>>> Matt<br>
>>>>><br>
>>>>>><br>
>>>>>> This flag is for optimization -- avoid MPI_Allreduce, which is expensive for np>10k.<br>
>>>>>> MatGetSubMatrices() works without this flag turns on.<br>
>>>>>> I just merged the improved MatGetSubMatrices_MPIAIJ() which only calls MPI_Allreduce once<br>
>>>>>> for repeated use. Previously, it calls MPI_Allreduce twice for each call of MatGetSubMatrices_MPIAIJ().<br>
>>>>>><br>
>>>>>> Hong<br>
>>>>>><br>
>>>>>> ______________________________<wbr>__________<br>
>>>>>> From: Barry Smith [<a href="mailto:bsmith@mcs.anl.gov">bsmith@mcs.anl.gov</a>]<br>
>>>>>> Sent: Friday, January 20, 2017 1:11 PM<br>
>>>>>> To: Matthew Knepley<br>
>>>>>> Cc: Zhang, Hong; PETSc<br>
>>>>>> Subject: Re: [petsc-dev] Bug introduced in MatGetSubmatrices()<br>
>>>>>><br>
>>>>>>> On Jan 20, 2017, at 12:45 PM, Matthew Knepley <<a href="mailto:knepley@gmail.com">knepley@gmail.com</a>> wrote:<br>
>>>>>>><br>
>>>>>>> On Fri, Jan 20, 2017 at 11:55 AM, Barry Smith <<a href="mailto:bsmith@mcs.anl.gov">bsmith@mcs.anl.gov</a>> wrote:<br>
>>>>>>><br>
>>>>>>>> On Jan 20, 2017, at 11:49 AM, Matthew Knepley <<a href="mailto:knepley@gmail.com">knepley@gmail.com</a>> wrote:<br>
>>>>>>>><br>
>>>>>>>> On Fri, Jan 20, 2017 at 10:49 AM, Hong <<a href="mailto:hzhang@mcs.anl.gov">hzhang@mcs.anl.gov</a>> wrote:<br>
>>>>>>>> Matt,<br>
>>>>>>>> By default, the flag C->submat_singleis = false.<br>
>>>>>>>> In PCSetUp_ASM(), we set it as 'true' to use MatGetSubMatrices_MPIAIJ_<wbr>SingleIS().<br>
>>>>>>>><br>
>>>>>>>> Can you check the value of this flag in your case?<br>
>>>>>>>><br>
>>>>>>>> The problem is the following:<br>
>>>>>>>><br>
>>>>>>>> 1) We use MatGetSubMatrices() to extract small matrices in order to form a preconditioner<br>
>>>>>>>><br>
>>>>>>>> 2) We do this at each Newton iteration<br>
>>>>>>>><br>
>>>>>>>> 3) We use ASM as a preconditioner for the eventual Newton solve<br>
>>>>>>>><br>
>>>>>>>> 4) The second time we call MatGetSubMatrices(), it has this flag set, even though we are using multiple ISes<br>
>>>>>>>><br>
>>>>>>>> Solution: ALSO check that the user is in fact passing a single IS.<br>
>>>>>>>><br>
>>>>>>> This requires a communication. I am confused, is the number of IS changing each time? If not why is the flag set?<br>
>>>>>>><br>
>>>>>>> ASM sets this flag because it knows that IT is going to call MatGetSubMatrices() later, but it unsafe if any user calls<br>
>>>>>>> MatGetSubMatrices() as well. I think overall its a fragile design and should be scrapped.<br>
>>>>>><br>
>>>>>> You mean the user calls the SAME MatGetSubMatrices with the same matrices, right? Not a completely different unrelated MatGetSubMatrices() which should not be affect by the previous unrelated call.<br>
>>>>>><br>
>>>>>><br>
>>>>>> Barry<br>
>>>>>><br>
>>>>>>><br>
>>>>>>> Matt<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>> Barry<br>
>>>>>>><br>
>>>>>>>> Matt<br>
>>>>>>>><br>
>>>>>>>> Hong<br>
>>>>>>>><br>
>>>>>>>> It comes from here:<br>
>>>>>>>><br>
>>>>>>>> <a href="https://bitbucket.org/petsc/petsc/commits/c10200c1442b553b7ad65c70101560db4fa22e78" rel="noreferrer" target="_blank">https://bitbucket.org/petsc/<wbr>petsc/commits/<wbr>c10200c1442b553b7ad65c70101560<wbr>db4fa22e78</a><br>
>>>>>>>><br>
>>>>>>>> If we ask for more than 1 matrix, it dispatches to<br>
>>>>>>>><br>
>>>>>>>> MatGetSubMatrices_MPIAIJ_<wbr>SingleIS()<br>
>>>>>>>><br>
>>>>>>>> but then fails here<br>
>>>>>>>><br>
>>>>>>>> <a href="https://bitbucket.org/petsc/petsc/annotate/2e559809f9aee9c95ee79eb0939630cfe5502c8d/src/mat/impls/aij/mpi/mpiov.c?at=master&fileviewer=file-view-default#mpiov.c-1306" rel="noreferrer" target="_blank">https://bitbucket.org/petsc/<wbr>petsc/annotate/<wbr>2e559809f9aee9c95ee79eb0939630<wbr>cfe5502c8d/src/mat/impls/aij/<wbr>mpi/mpiov.c?at=master&<wbr>fileviewer=file-view-default#<wbr>mpiov.c-1306</a><br>
>>>>>>>><br>
>>>>>>>> because ismax > 1. I think the ismax check needs to move up to here<br>
>>>>>>>><br>
>>>>>>>> <a href="https://bitbucket.org/petsc/petsc/annotate/2e559809f9aee9c95ee79eb0939630cfe5502c8d/src/mat/impls/aij/mpi/mpiov.c?at=master&fileviewer=file-view-default#mpiov.c-2012" rel="noreferrer" target="_blank">https://bitbucket.org/petsc/<wbr>petsc/annotate/<wbr>2e559809f9aee9c95ee79eb0939630<wbr>cfe5502c8d/src/mat/impls/aij/<wbr>mpi/mpiov.c?at=master&<wbr>fileviewer=file-view-default#<wbr>mpiov.c-2012</a><br>
>>>>>>>><br>
>>>>>>>> but I don't know for sure. Please fix this since it is breaking PyLith.<br>
>>>>>>>><br>
>>>>>>>> Matt<br>
>>>>>>>><br>
>>>>>>>> --<br>
>>>>>>>> What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.<br>
>>>>>>>> -- Norbert Wiener<br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>> --<br>
>>>>>>>> What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.<br>
>>>>>>>> -- Norbert Wiener<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>><br>
>>>>>>><br>
>>>>>>> --<br>
>>>>>>> What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.<br>
>>>>>>> -- Norbert Wiener<br>
>>>>>><br>
>>>>>><br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>>> --<br>
>>>>> What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.<br>
>>>>> -- Norbert Wiener<br>
>>>><br>
>>>><br>
>>>><br>
>>>><br>
>>>> --<br>
>>>> What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.<br>
>>>> -- Norbert Wiener<br>
>>><br>
>>><br>
>><br>
>><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div>