[petsc-dev] ISRestoreNonlocalIS?

[Dmitry Karpeev]

On Thu, Nov 18, 2010 at 3:18 PM, Jed Brown <jed at 59a2.org> wrote:
> On Thu, Nov 18, 2010 at 21:44, Barry Smith <bsmith at mcs.anl.gov> wrote:
>>
>> Cacheing information about the parallel structure of the IS input into
>> MatGetSubMatrix() inside the gotten matrix is IMHO not a good model (my god,
>> what if someone passed in a different IS next time? just a joke).
>
> I understand that there needs to be a less ad-hoc consistency model, but how
> are you going to check that the user didn't pass in a different IS, maybe of
> different size and maybe not?  Seems like the old IS still needs to be
> cached in some form, so you can check whether it has changed.

The old IS, defining the rows or columns of the submatrix is passed in
to MatGetSubMatrices
every time.  In a sense, the user caches it.  The corresponding
ISGetNonlocalIS uses the information
cached inside the row/col (actually, col only) IS.

>>
>> Here is the model we are proposing. An IS is a collection of indices,
>> often stored in parallel. Some operations on IS's only require information
>> about the local part of the indices, other operations require information
>> about parts of the indices on other processes. Ok, that information about
>> remote indices can be obtained through a collective operation. Sometimes one
>> may want that same information about the remote part several different
>> times, well it could do a collective each time or it could cache on the
>> first time and reuse later (whether it caches or recommunicates is internal
>> to the object and merely an optimization and need not be actively visible to
>> the user.) The previous solution to this need for remote information was to
>> use an ISAllGather() and stash that information "somewhere else" for later
>> use (and it is currently stashed elsewhere as a SEQUENTIAL IS), but since
>> that information is ONLY information about that particular IS I submit that
>> any stashing (should it occur) should be inside the IS not inside some other
>> IS object.
>
> I am still skeptical that ISAllGather is ever really required, except as a
> convenience.  I realize that figuring out who to communicate with in
> MatGetSubMatrices is (even) messier without it, but there must be a more
> scalable way to implement it.  Especially when the overlap is local.

I'm not disagreeing, but I also don't see a general way of figuring
out the off-rank
columns for a submatrix that is destined to live on a subcomm.
It's possible that that information has to come from on high -- the
mesh or some
other object that has information inaccessible to the linear algebra.
In that case, however, the user would have to pass in two columns ISs
-- the local
and the nonlocal submatrix columns.

Another point is that since we anticipate a submatrix is extracted on a subcomm,
the size of the submatrix and the subcomm might not grow in the weak
scaling case.


> There is no VecGetTotalArray(), just sayin'.

The names are definitely an stop-gap fix: for consistency we'd have to change
ISGetIndices -->  ISGetLocalIndices, ISGetTotalIndices -->
ISGetIndices, VecGetArray --> VecGetLocalArray, etc.
Maybe this will be done one day :-)

>
>>
>> > What is going on here?  If IS is caching this nonlocal stuff, then every
>> > function causing mutation (e.g. ISGeneralSetIndices, and a few others) needs
>> > to be visited and made to discard the gathered and nonlocal caches.
>>
>>    This is just a coding detail and could be added.  But actually we
>> should probably decide once and for all if IS's are mutable, I tend to think
>> they shouldn't be but we are not completely consistent yet.
>
> I would be happy with always immutable.  The only tricky thing is the
> in-place ISSort() that can operate on const data that the user passed in
> with ISGeneralCreate().  I'm still waiting for someone to pass in an array
> from a read-only page.  Sure, making a copy is not necessary for the
> algorithm, but I think the simplicity of a strictly immutable interface
> would be worth it.
>
>>
>> > I just don't understand the point of all these new functions, they feel
>> > like an inferior interface to something that is currently available.
>>
>>   They are a proper replacement to a previous inferior interface that
>> creates this strange new IS from a given IS. All the new stuff is is a API
>> for accessing nonlocal parts of the IS efficiently and consistently and
>> simply.
>
> Could we at least get rid of ISGetTotalIndices() and ISGetNonlocalIndices()
> in favor of ISGetNonlocalIS() followed by ISGetIndices(), so that the
> interface allows working with ISStride and ISBlock efficiently?
>
>>
>>    For now ISAllGather() still exists, but in the long run I think we will
>> be able to toss it.
>
> Or rename it ISGetTotalIS() and make it cached by the IS.  For one-off
> (often preprocessing) use of gathered indices, perhaps there should be an
> advanced function ISCollectGarbage (or PetscObjectCollectGarbage) which
> could free the expensive gathered indices (and matrices used inside
> MatGetSubMatrix).
>
>>
>> > Same question for ISAllGatherIndices which has been around for a long
>> > time.  It looks to me like the user can do ISCreateGeneral(), ISAllGather(),
>> > ISGetIndices() to achieve exactly the same thing (no performance
>> > difference).  Why even call it IS when no IS (or PETSc object of any kind is
>> > used by this function)?
>>
>>   It is called an IS because it involves the manipulation of sets of
>> indices which is what IS does. It is a static method for the IS class :-)
>
> It seems a lot like the MatGetSubMatrixRaw that you were so offended by.
> Jed

Dmitry.