I don’t know if you really meant to ask for José's opinion here, but I personally think that releasing all 3.13.X version with a broken MatMatMult and no deprecation warning concerning MATOP_MAT_MULT is not the best. Thanks, Pierre
> On 23 Apr 2020, at 4:03 AM, Zhang, Hong <hzh...@mcs.anl.gov> wrote: > > Jose, > I'll check and fix them. I have to do it in master, is ok? > Hong > > From: Pierre Jolivet <pierre.joli...@enseeiht.fr> > Sent: Wednesday, April 22, 2020 3:08 PM > To: Zhang, Hong <hzh...@mcs.anl.gov> > Cc: Jose E. Roman <jro...@dsic.upv.es>; Stefano Zampini > <stefano.zamp...@gmail.com>; petsc-dev <petsc-dev@mcs.anl.gov>; Smith, Barry > F. <bsm...@mcs.anl.gov> > Subject: Re: [petsc-dev] MATOP_MAT_MULT > > Hong, > I also now just tested some previously PETSC_VERSION_LT(3,13,0) running code > with C=A*B, Dense=Nest*Dense, all previously allocated prior to a call to > MatMatMult and scall = MAT_REUSE_MATRIX. > Sadly, it’s now broken. It is my fault for not having a test for this in > https://gitlab.com/petsc/petsc/-/merge_requests/2069 > <https://gitlab.com/petsc/petsc/-/merge_requests/2069>, sorry about that. > [0]PETSC ERROR: Call MatProductSymbolic() first > [0]PETSC ERROR: #1 MatProductNumeric() line 730 in > /ccc/work/cont003/rndm/rndm/petsc/src/mat/interface/matproduct.c > [0]PETSC ERROR: #2 MatMatMult() line 9335 in > /ccc/work/cont003/rndm/rndm/petsc/src/mat/interface/matrix.c > > Here is a reproducer (that will work OK with 3.12.4). > diff --git a/src/mat/tests/ex195.c b/src/mat/tests/ex195.c > index c72662bc3c..811de669c5 100644 > --- a/src/mat/tests/ex195.c > +++ b/src/mat/tests/ex195.c > @@ -73,2 +73,3 @@ int main(int argc,char **args) > ierr = MatMatMult(nest,B,MAT_REUSE_MATRIX,PETSC_DEFAULT,&C);CHKERRQ(ierr); > + ierr = MatMatMult(nest,C,MAT_REUSE_MATRIX,PETSC_DEFAULT,&B);CHKERRQ(ierr); > ierr = MatMatMultEqual(nest,B,C,10,&equal);CHKERRQ(ierr); > > $ make -f gmakefile test searchin=mat_tests-ex195 > > I believe this is very close to the topic at hand and issue #608, so maybe > you could fix this as well in the same upcoming MR? Just let me know, I can > have a crack it otherwise. > Thanks, > Pierre > >> On 22 Apr 2020, at 5:38 PM, Zhang, Hong <hzh...@mcs.anl.gov >> <mailto:hzh...@mcs.anl.gov>> wrote: >> >> Jose, Pierre and Stefano, >> Now I understand the issue that Stefano raised. I plan to add >> MatProductIsSupported(Wmat,&supported,&matproductsetfromoptions) >> the flag 'supported' tells if the product is supported/implemented or not, >> and the function pointer 'matproductsetfromoptions' gives the name of >> MatProductSetFromOptions_xxx, (including basic implementation) or NULL. >> >> Let me know your suggestions. I'll list all of you as reviewer. >> Hong >> >> >> From: Jose E. Roman <jro...@dsic.upv.es <mailto:jro...@dsic.upv.es>> >> Sent: Wednesday, April 22, 2020 9:07 AM >> To: Stefano Zampini <stefano.zamp...@gmail.com >> <mailto:stefano.zamp...@gmail.com>> >> Cc: Zhang, Hong <hzh...@mcs.anl.gov <mailto:hzh...@mcs.anl.gov>>; Pierre >> Jolivet <pierre.joli...@enseeiht.fr <mailto:pierre.joli...@enseeiht.fr>>; >> petsc-dev <petsc-dev@mcs.anl.gov <mailto:petsc-dev@mcs.anl.gov>> >> Subject: Re: [petsc-dev] MATOP_MAT_MULT >> >> I agree with Pierre and Stefano. >> Hong: your proposed solution would be fine, but MATOP_MATPRODUCT does not >> exist yet, so I cannot try it. >> I would like a solution along the lines of what Stefano suggests. It is not >> too much trouble if it goes to master instead of maint. >> >> Thanks. >> Jose >> >> >> > El 22 abr 2020, a las 15:26, Stefano Zampini <stefano.zamp...@gmail.com >> > <mailto:stefano.zamp...@gmail.com>> escribió: >> > >> > >> >> >> >> MatProductCreateWithMat(A,Vmat,NULL,Wmat); >> >> MatProductSetType(Wmat,MATPRODUCT_AB); >> >> MatHasOperation(Wmat,MATOP_MATPRODUCT,&flg); //new support, it calls >> >> MatProductSetFromOptions(Wmat) >> > >> > Hong, this would go in the direction I was outlining here >> > https://gitlab.com/petsc/petsc/-/issues/608 >> > <https://gitlab.com/petsc/petsc/-/issues/608> >> > How about also adding something like >> > >> > MatProductIsImplemented(Wmat,&flg) >> > >> > That returns true if a specific implementation is available? This way >> > >> > This way, if we use both queries, we can assess the presence of the basic >> > fallbacks too, i.e. >> > >> > MatHasOperation(Wmat,MATOP_MATPRODUCT,&flg1) >> > MatProductIsImplemented(Wmat,&flg2) >> > >> > If flg1 is false, no support at all >> > If flg1 is true and flg2 is false -> Basic implementation (i.e, MatShell >> > with products inside) >> > If flg1 and flg2 are both true -> Specific implementation available. >> > >> >> if (V->vmm && flg) { >> >> MatProductSymbolic(Wmat); >> >> MatProductNumeric(Wmat); >> >> } else { >> >> MatDestroy(Wmat); >> >> ... >> >> } >> >> Hong >> >> >> >> >> >> From: Jose E. Roman <jro...@dsic.upv.es <mailto:jro...@dsic.upv.es>> >> >> Sent: Tuesday, April 21, 2020 11:21 AM >> >> To: Pierre Jolivet <pierre.joli...@enseeiht.fr >> >> <mailto:pierre.joli...@enseeiht.fr>> >> >> Cc: Zhang, Hong <hzh...@mcs.anl.gov <mailto:hzh...@mcs.anl.gov>>; >> >> petsc-dev <petsc-dev@mcs.anl.gov <mailto:petsc-dev@mcs.anl.gov>> >> >> Subject: Re: [petsc-dev] MATOP_MAT_MULT >> >> >> >> >> >> >> >> > El 21 abr 2020, a las 17:53, Pierre Jolivet <pierre.joli...@enseeiht.fr >> >> > <mailto:pierre.joli...@enseeiht.fr>> escribió: >> >> > >> >> > >> >> > >> >> >> On 21 Apr 2020, at 5:22 PM, Zhang, Hong <hzh...@mcs.anl.gov >> >> >> <mailto:hzh...@mcs.anl.gov>> wrote: >> >> >> >> >> >> Pierre, >> >> >> MatMatMult_xxx() is removed from MatOps table. >> >> > >> >> > Shouldn’t there be a deprecation notice somewhere? >> >> > There is nothing about MATOP_MAT_MULT in the 3.13 changelog >> >> > https://www.mcs.anl.gov/petsc/documentation/changes/313.html >> >> > <https://www.mcs.anl.gov/petsc/documentation/changes/313.html> >> >> > For example, I see that in SLEPc, José is currently making these >> >> > checks, which are in practice useless as they always return >> >> > PETSC_FALSE?https://gitlab.com/slepc/slepc/-/blob/master/src/sys/classes/bv/impls/contiguous/contig.c#L191 >> >> > >> >> > <https://gitlab.com/slepc/slepc/-/blob/master/src/sys/classes/bv/impls/contiguous/contig.c#L191> >> >> > (Maybe José is aware of this and this is just for testing) >> >> >> >> No, I was not aware of this. Thanks for bringing this up. Now in 3.13 we >> >> are always doing the slow version (column by column), so yes I am >> >> interested in a solution for this. >> >> >> >> > >> >> >> MatMatMult() is replaced by >> >> >> MatProductCreate() >> >> >> MatProductSetType(,MATPRODUCT_AB) >> >> >> MatProductSetFromOptions() >> >> >> MatProductSymbolic() >> >> >> MatProductNumeric() >> >> >> >> >> >> Where/when do you need query a single matrix for its product operation? >> >> > >> >> > I didn’t want to bother at first with the new API, because I’m only >> >> > interested in C = A*B with C and B being dense. >> >> > Of course, I can update my code, but if I understand Stefano’s issue >> >> > correctly, and let’s say my A is of type SBAIJ, for which there is no >> >> > MatMatMult, the code will now error out in the MatProduct? >> >> > There is no fallback mechanism? Meaning I could in fact _not_ use the >> >> > new API and will just have to loop on all columns of B, even for AIJ >> >> > matrices. >> >> > >> >> > Thanks, >> >> > Pierre >> >> > >> >> >> Hong >> >> >> >> >> >> From: petsc-dev <petsc-dev-boun...@mcs.anl.gov >> >> >> <mailto:petsc-dev-boun...@mcs.anl.gov>> on behalf of Pierre Jolivet >> >> >> <pierre.joli...@enseeiht.fr <mailto:pierre.joli...@enseeiht.fr>> >> >> >> Sent: Tuesday, April 21, 2020 7:50 AM >> >> >> To: petsc-dev <petsc-dev@mcs.anl.gov <mailto:petsc-dev@mcs.anl.gov>> >> >> >> Subject: [petsc-dev] MATOP_MAT_MULT >> >> >> >> >> >> Hello, >> >> >> Am I seeing this correctly? >> >> >> #include <petsc.h> >> >> >> >> >> >> int main(int argc,char **args) >> >> >> { >> >> >> Mat A; >> >> >> PetscBool hasMatMult; >> >> >> PetscErrorCode ierr; >> >> >> >> >> >> ierr = PetscInitialize(&argc,&args,NULL,NULL);if (ierr) return ierr; >> >> >> ierr = MatCreate(PETSC_COMM_WORLD,&A);CHKERRQ(ierr); >> >> >> ierr = MatSetType(A,MATMPIAIJ);CHKERRQ(ierr); >> >> >> ierr = MatHasOperation(A,MATOP_MAT_MULT,&hasMatMult);CHKERRQ(ierr); >> >> >> printf("%s\n", PetscBools[hasMatMult]); >> >> >> ierr = PetscFinalize(); >> >> >> return ierr; >> >> >> } >> >> >> >> >> >> => FALSE >> >> >> >> >> >> I believe this is a regression (or at least an undocumented change) >> >> >> introduced here:https://gitlab.com/petsc/petsc/-/merge_requests/2524/ >> >> >> <https://gitlab.com/petsc/petsc/-/merge_requests/2524/> >> >> >> I also believe Stefano raised a similar point there: >> >> >> https://gitlab.com/petsc/petsc/-/issues/608 >> >> >> <https://gitlab.com/petsc/petsc/-/issues/608> >> >> >> This is a performance killer in my case because I was previously using >> >> >> this check to know whether I could use MatMatMult or had to loop on >> >> >> all columns and call MatMult on all of them. >> >> >> There is also a bunch of (previously functioning but now) broken code, >> >> >> e.g.,https://www.mcs.anl.gov/petsc/petsc-current/src/mat/impls/transpose/transm.c.html#line105 >> >> >> >> >> >> <https://www.mcs.anl.gov/petsc/petsc-current/src/mat/impls/transpose/transm.c.html#line105>or >> >> >> >> >> >> https://www.mcs.anl.gov/petsc/petsc-current/src/mat/impls/nest/matnest.c.html#line2105 >> >> >> >> >> >> <https://www.mcs.anl.gov/petsc/petsc-current/src/mat/impls/nest/matnest.c.html#line2105> >> >> >> Is this being addressed/documented? >> >> >> >> >> >> Thanks, >> >> >> Pierre >> >> > >> >