I agree with Pierre. However, if the fix involves an API change then I could understand it goes to master.
> El 23 abr 2020, a las 7:43, Pierre Jolivet <pierre.joli...@enseeiht.fr> > escribió: > > 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, 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> 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> >>> Sent: Wednesday, April 22, 2020 9:07 AM >>> To: Stefano Zampini <stefano.zamp...@gmail.com> >>> Cc: Zhang, Hong <hzh...@mcs.anl.gov>; Pierre Jolivet >>> <pierre.joli...@enseeiht.fr>; petsc-dev <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> >>> > 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 >>> > 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> >>> >> Sent: Tuesday, April 21, 2020 11:21 AM >>> >> To: Pierre Jolivet <pierre.joli...@enseeiht.fr> >>> >> Cc: Zhang, Hong <hzh...@mcs.anl.gov>; petsc-dev <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> escribió: >>> >> > >>> >> > >>> >> > >>> >> >> On 21 Apr 2020, at 5:22 PM, Zhang, Hong <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 >>> >> > 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 >>> >> > (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> on behalf of Pierre >>> >> >> Jolivet <pierre.joli...@enseeiht.fr> >>> >> >> Sent: Tuesday, April 21, 2020 7:50 AM >>> >> >> To: petsc-dev <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/ >>> >> >> I also believe Stefano raised a similar point there: >>> >> >> 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#line105or >>> >> >> >>> >> >> https://www.mcs.anl.gov/petsc/petsc-current/src/mat/impls/nest/matnest.c.html#line2105 >>> >> >> Is this being addressed/documented? >>> >> >> >>> >> >> Thanks, >>> >> >> Pierre >>> >> > >>> > >
