[petsc-dev] I do not think this is the right solution

[Barry Smith]

On Mar 18, 2012, at 6:24 PM, Matthew Knepley wrote:

> On Sun, Mar 18, 2012 at 5:47 PM, Barry Smith <bsmith at mcs.anl.gov> wrote:
> 
> > Let me clarify. I did not find your completely undocumented "feature" that has "always been that way". I found the places where
> > values would have to be communicated, which is what the documentation says the assembly does.
> 
>    Here is the documentation of the two flags. In no way shape or form does it say anything about these flags in relation to communication!
> 
>  PetscBool              assembled;        /* is the matrix assembled? */
>  PetscBool              was_assembled;    /* new values inserted into assembled mat */
> 
>   Here is the documentation for MatAssemblyBegin/End  Again this does not say that this function was in anyway only connected to communication
> 
>   MatAssemblyBegin - Begins assembling the matrix.  This routine should
>   be called after completing all calls to MatSetValues().
> 
>  You made an assumption about the flags that was not correct, there is more to sticking values into the matrices then the communication and these two flags have always handled everything not just an indication of communication needed.
> 
> > Here is what I do not understand. Isn't it obvious when an insertion changes nonzero structure? Shouldn't we fix this obvious functionality
> > hole rather than complicating the interface?
> 
>   PETSc was written with the separated setvalues phase and  usematrix/accessvalues phases, this is clearly documented and permeates the use of Mat and has always been fundamental to the design and usage of PETS. Suddenly YOU decide to change this model, on your own, and do a half-assed job of it in the process.  Did you even run the complete test suite and make sure your change didn't break anything?
> 
>   We could change the model (though I think changing it is a bad idea) but being such a fundamental change it should have been discussed and if changed the users manual, the tutorials, and examples would all need major revision to describe the new model (which is actually more complicated than the old model since the meaning of MatSetValues() would be different depending on the current state of the matrix.)
> 
>   So let's discuss changing the model. In my model the setvalues phase the usematrix/accessvalues phase must always be separated with MatAssemblyBegin/End. In Matt's model one can apparently call MatSetValues() sometimes after an assembly and then not call MatAssemblyBegin/End() so
> 
> In the alternate model, you call MatAssemblyBegin/End() when the matrix is marked as unassembled. Its easy to check this flag, so the user
> will not be confused. Barry is correct that the flag reset is not collective, so that query would have to be.
>  
> long as one does not do communication? Or is it change the nonzero pattern? What if some processes do not change the nonzero pattern but others do, well then the user has to know this case and have all processes call MatAssemblyBegin/End() everywhere if any process changes the pattern (for example if one process changes its off-diagonal pattern no communication is needed to move value, but ALL processes must cooperate in setting up the new scatter for the multiply, this is normally handled by the required MatAssemblyBegin/End() process? Your model is fragile and confusing, yes it could be made to work but I think is not as good a model.
> 
> I do not think "fragile" is an accurate characterization because failure is immediate when trying to use a function that requires assembly, and it
> also obvious what the fix is, to assemble the matrix.
> 
> You might mean that the existing model is more intuitive. I disagree since it is currently impossible to do something simple like
> chop out small values from a matrix.

   Using MatSetValues() to set those locations to zero is fine, but so what, really the reason to chop out small values would be to also remove the nonzero locations from the matrix which your model cannot do anyway and would likely require methods for each matrix format directly. Yes, the current model is not good for allowing people to manipulate matrix entries directly but I don't really care, that is not what PETSc is for.


> The proposal is to augment the current model with MatGetRowWrite(). This would mean
> not only adding to the interface, but writing all the code to put values back into the matrix if the row is a copy. I think it is more
> intuitive to allow the user to SetValues() if the nonzero pattern is unchanged and no values need to be communicated.

   I do not trust the users to use this alternative use of MatSetValues() correctly.  I won't even add the MatGetRowWrite() method unless a useful use case appears, certainly zeroing small values is not such a case.

   Barry

> 
> However, I do not care enough if no one else thinks its easier to have one smarter method for value insertion, than two more
> restrictive methods.
> 
>     Matt
>  
>   Barry
> 
> 
> 
> On Mar 18, 2012, at 3:07 PM, Matthew Knepley wrote:
> 
> > On Sun, Mar 18, 2012 at 2:49 PM, Barry Smith <bsmith at mcs.anl.gov> wrote:
> >
> > On Mar 18, 2012, at 12:09 PM, Matthew Knepley wrote:
> >
> > > On Sun, Mar 18, 2012 at 11:52 AM, Barry Smith <bsmith at mcs.anl.gov> wrote:
> > >
> > > On Mar 18, 2012, at 11:39 AM, Matthew Knepley wrote:
> > >
> > > > https://bitbucket.org/petsc/petsc-dev/changeset/7ae47bfbd6e9
> > > >
> > > > Why are we using the assembled flag to determine whether garray and colmap
> > > > should be created? That seems like an abuse of the flag. Shouldn't we check
> > > > for these when we need them?
> > >
> > >   This has always been the use of that flag since day one. That is the reason for the existence of that flag. Now you want to change the meaning of the flag.
> > >
> > >   Since your MatGetRow() MatSetValues() paradigm only works properly if no new nonzeros are introduced (otherwise people will get assembled errors after they've put in some values) what is wrong with my proposed solution of MatGetRowWrite() which is the correct interface for changing some values in the row but not the nonzero structure.
> > >
> > > Your own metric. It complicates the interface. I have to know that I only want to change existing nonzeros, when it is
> > > perfectly easy for SetValues() to detect this AND I already went and found all the places where this happens.
> >
> >   Found all the places where what happens?  You introduced a bug, for example mat/examples/tests/ex37.c with multiple processes crashed after you "found all the places where this happens".  I wasted hours tracking down the problem from you missing "one of the places where this happens". And how come you didn't change all the other MatSetValuesXXX() that also use the same assembled flags? I don't think you came close to "found all the places where this happened".
> >
> > Let me clarify. I did not find your completely undocumented "feature" that has "always been that way". I found the places where
> > values would have to be communicated, which is what the documentation says the assembly does.
> >
> >  PETSc Mat has ALWAYS had a simple well-defined policy of a phase where you set values in the matrix and a separate phase where you can access values; it has served us very well over the years. Now you suddenly what to change this policy "sometimes".
> >
> >   I also think you misunderstand my proposal that SAFELY gives you everything you want in a clean way that does not confuse the setvalues phase and the accessvalues phase.
> >
> >    MatGetRowWrite(mat,PetscInt row,PetscInt *nz, const PetscInt **indices,PetscScalar **values)
> >    /*   here I have code that changes some of the entries in values; note I cannot change the indices since they are const */
> >     MatRestoreRowWrite(mat,PetscInt row,PetscInt *nz, const PetscInt **indices,PetscScalar **values)  the routine puts the changed values back into the underly Mat. no chance of changing nonzero structure.
> >
> >    With your model
> >
> >    MatGetRow(mat,PetscInt row,PetscInt *nz, const PetscInt **indices,const PetscScalar **values)
> >     MatRestoreRow(mat,PetscInt row,PetscInt *nz, const PetscInt **indices,const PetscScalar **values)
> >    MatSetValues(mat,row ....)
> >
> >     there is no mechanism to insure that the user only sets values that already existed in the matrix which may trigger a change in the nonzero structure which means the next call to MatGetRow() will fail. Plus it posses the problem why can you sometimes mix accessing values and calling MatSetValues() but sometimes not? Having one simple policy on separating the phases is valuable and should be kept.
> >
> > Here is what I do not understand. Isn't it obvious when an insertion changes nonzero structure? Shouldn't we fix this obvious functionality
> > hole rather than complicating the interface?
> >
> > I just changed the code I had to Assemble() all over the place.
> >
> >    Matt
> >
> >   Barry
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >    Barry
> >
> > >
> > >    Matt
> > >
> > >
> > >   Barry
> > >
> > > >
> > > >    Matt
> > > >
> > > > --
> > > > What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.
> > > > -- Norbert Wiener
> > >
> > >
> > >
> > >
> > > --
> > > What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.
> > > -- Norbert Wiener
> >
> >
> >
> >
> > --
> > What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.
> > -- Norbert Wiener
> 
> 
> 
> 
> -- 
> What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead.
> -- Norbert Wiener