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
>> >> > 
>> >

Reply via email to