[petsc-dev] DMDA_*PERIODIC and DMDA_XYZGHOSTED

Barry Smith bsmith at mcs.anl.gov
Thu Mar 10 14:57:30 CST 2011 

  Satish,

   Please drop them in and we'll see what breaks.

  Ethan,

    Did you update src/docs/website/documentation/changes/dev.html to reflect the API change? If not please do so and send Satish directly that patch.


  Barry


On Mar 10, 2011, at 2:30 PM, Ethan Coon wrote:

> Attached is a patchq for enabling DMDA_*GHOSTED to be used, in which
> case ghost nodes are included even at domain boundaries of nonperiodic
> dimensions.  The DMDAPeriodicType enum was redone, but includes
> DMDA_XYPERIODIC/etc to make it compatible with the old version of the
> enum.  To use, simply compose the DMDAPeriodicType with bitwise or:
> 
> DMDA_XPERIODIC | DMDA_YGHOSTED, for example.
> 
> This just slightly contributes to the ugliness of the 2d/3d SetUp
> methods, but not much.  The only place I've introduced negative indices
> at this point is into the DMLocalToGlobalMapping, where there must be
> global numbers for the local ghost cells which don't exist in the global
> vec.  Allowing negative indices to the VecScatters would simplify a lot
> of this code, but I don't really have time to take on another
> fundamental piece of PETSc at this point...
> 
> In the process, I've cleaned up all the old stuff where the index sets
> were generated in true "dof-strided" indices (instead of block indices),
> and removed all the code to patch in that change to ISCreateBlock().  At
> the end, all the x-component pieces of the DMDA are multiplied by the #
> of dofs, as it seems much of PETSc depends upon this (though it wasn't
> clear why except if for historical reasons).
> 
> All the dm/examples/tests pass, and I redid the scripts in
> dm/examples/tests/scripts so that they both worked and test all
> combinations of periodicities/ghosted and stencils.  Some regression
> tests of my own code work as well.  I checked the Interp operators for
> MG, and they look fine (and seem to pass existing regression tests), but
> that could use some checking.  Let me know if I'm missing another set of
> tests that I should be running... not sure what the standards are for
> contributions like this.
> 
> Thanks,
> 
> Ethan
> 
> 
> 
> On Wed, 2011-03-02 at 18:33 -0600, Barry Smith wrote:
>> On Mar 2, 2011, at 6:11 PM, Ethan Coon wrote:
>> 
>>> On Wed, 2011-03-02 at 16:26 -0600, Barry Smith wrote:
>>>> On Mar 2, 2011, at 1:47 PM, Ethan Coon wrote:
>>>> 
>>>>> 
>>>>>> 
>>>>>> Ethan,
>>>>>> 
>>>>>> MatSetValues() and VecSetValues() handle negative indices as "ignore these entries".  Currently VecScatterCreate() does not handle negative indices as "ignore these entries" (at least it is not documented and I did not write it), likely it will either crash or generate an error. 
>>>>>> 
>>>>>> It sounds like you are proposing that if the from or to entry in a particular "slot" is negative you would like VecScatterCreate() to just ignore that slot? This seems like an ok proposal if you are willing to update VecScatterCreate() to handle it and add to VecScatterCreate() manual page this feature. If this truly simplifies all the horrible if () code in the DA construction to handle corner stuff then it would be worth doing.
>>>>>> 
>>>>> 
>>>>> Hmm, I was proposing that, but because I thought that was the case
>>>>> already.  
>>>>> 
>>>>> It would clean up the DASetUp code, but not as much as I thought
>>>>> initially.  Currently VecSetValuesLocal(), when used with the L2G
>>>>> mapping from a STAR_STENCIL DMDA, will happily add/insert values from
>>>>> the ghost cells in the corner to the global vector (why you would add
>>>>> values to a ghost node on which you don't want to get information back
>>>>> from I don't know, but someone made an effort to implement it...).
>>>> 
>>>>  I think that is a "bug" or "unintended feature" I hope nobody worked hard to get it to work.  I think it is just that way because that is the way it worked out. Probably DMGetMatrix_DA() should call MatSetOption(mat,MAT_NO_NEW_NONZEROS,PETSC_TRUE); to prevent people of accidently using those slots (if they really want to for some perverse reason they could call MatSetOption() themselves to reset it.
>>>> 
>>>>  Barry
>>>> 
>>>>> To
>>>>> keep that feature, the L2GMapping must be different from the IS used for
>>>>> the DMDAGlobalToLocal scatter, and all the ugly if crap has to stay.
>>>> 
>>>> Even in the star case the L2G has to contain slots for those stencil points (though you could fill those slots with negative entries I guess) to get the VecSetValuesLocal() to work. Is that what you propose, putting negative numbers there?? Seems possibly ok to me.  The one danger is that if they user intends to set that corner value with VecSetValuesLocal() or MatSetValuesLocal() it will be silently discarded (of course they should never try to set it but they may).
>>>> 
>>>> 
>>>> 
>>>>  Barry
>>>> 
>>> 
>>> Right, got it, there has to be local number for the gxs,gys,gzs entry
>>> even in the star case.  The question is if it has to get mapped
>>> someplace.  
>>> 
>>> Using VecSetValuesLocal() on an entire local domain with ADD_VALUES, the
>>> current implementation will sum the entries from ghost nodes and
>>> interior nodes, while in the INSERT_VALUES, it would lead to a race
>>> condition, where the processor who owns the value may not win (and it
>>> does so silently).  (Note this is true for any stencil and any ghost
>>> node, not just corners in the star stencil.)
>> 
>>  Using INSERT_VALUES with VecSetValues() also as well as the MatSetValues versions always has the condition that it is undefined who wins when several processes try to put in the same location. This is just a fact of (PETSc) life. I don't think DM or SetValuesLocal() really makes it any worse.
>> 
>> 
>>> 
>>> If instead we put -1 in all ghost indices of the l2g mapping, then
>>> VecSetValuesLocal() with INSERT_VALUES functions as expected (the value
>>> in the global vec is the value given by the process owning the node),
>>> while ADD_VALUES will only add in the value given by the process owning
>>> the node (and do so silently).  This behavior for ADD_VALUES is exactly
>>> what is described in the "notes" section of DMLocalToGlobalBegin's man
>>> page.  Basically it just doesn't allow you to VecSetLocalValues() on
>>> ghost nodes.
>>> 
>>> Compare this to the result of the operation: DMDAVecGetArray(da, local),
>>> assignment to the array, restore the array, then call
>>> DMDALocalToGlobal().  With DMDALocalToGlobal() and INSERT_VALUES, it
>>> does the l2g scatter, and so there is no race condition and the global
>>> vec gets the value in the array of the process owning that entry.  With
>>> ADD_VALUES, it does the reverse of the g2l scatter, so all values get
>>> added in.  
>>> 
>>> I guess I'm more concerned about the undocumented race condition than
>>> not adding values from ghost cells, which could easily be explained in a
>>> man page.  So yes, I'm proposing to put -1 as the global index of all
>>> ghost nodes in the local to global mapping.  
>>> 
>>> Note that I have to put something in the ghosted (nonperiodic) local
>>> number spots as well -- they have no corresponding global number, so it
>>> has to be -1.  At least this is then consistent that all ghost nodes in
>>> a VecSetValuesLocal() get ignored.  And if you really want to do this,
>>> it's likely you're using the (safer) DMLocalToGlobal() way anyway.
>>> 
>> 
>>   Go ahead and add support for VecScatterCreate() to handle negative indices and simplify (greatly) the DACreates if you are up for it.
>> 
>>   Thanks
>> 
>>   Barry
>> 
>> 
>>> Ethan
>>> 
>>> 
>>>>> I can just graft the Ghosted case on to that code (making it only
>>>>> slightly more ugly).  It will still depend upon the VecSetValues()
>>>>> accepting and ignoring negative global indices, but that's already the
>>>>> case.
>>>>> 
>>>>> Ethan
>>>>> 
>>>>>> Performance is not an issue since you would just discard those slots in the VecScatterCreate() phase and they would never appear in the actual scatter operations.
>>>>>> 
>>>>>> 
>>>>>> Barry
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> Ethan
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On Tue, 2010-12-07 at 15:41 -0700, Ethan Coon wrote:
>>>>>>>> On Tue, 2010-12-07 at 13:45 -0600, Barry Smith wrote:
>>>>>>>>> DMDA_XYZGHOSTED does not exist for 2d and 3d it was added, I'm guessing, as an experiment and was never in the initial design of DMDA. To fully support it one needs to go back tot he design of DMDA and see how to have it properly done and not just bolt it on.  Some people like to use these types of ghost nodes so I agree it is a useful thing to have but who is going to properly add it?
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> At some point in the not-too-distant future I'll get frustrated enough
>>>>>>>> to look into this, but I don't have the time at the moment.  At first
>>>>>>>> glance it looks like:
>>>>>>>> 
>>>>>>>> - Ensure DMDA{X,Y,Z}Periodic() macros are used everywhere instead of
>>>>>>>> direct comparisons to dd->wrap (they aren't used everywhere currently).
>>>>>>>> 
>>>>>>>> - Define macros DMDA{X,Y,Z}Ghosted() to (in some places) replace
>>>>>>>> DMDA{X,Y,Z}Periodic() and then choosing the appropriate macro in the
>>>>>>>> right places.
>>>>>>>> 
>>>>>>>> - This probably doesn't merit a change in the DMDACreate* API (it would
>>>>>>>> affect a very large amount of user code).  The most obvious alternative
>>>>>>>> to an API change would be a larger, somewhat convoluted enum for the
>>>>>>>> PeriodicType (DMDA_XPERIODIC_YGHOSTED, DMDA_XYGHOSTED, etc) which could
>>>>>>>> at least be made backward compatible.
>>>>>>>> 
>>>>>>>> At least all of the functionality should be there already (since it's
>>>>>>>> needed in the periodic case)... it's just higher level code that would
>>>>>>>> need to change.
>>>>>>>> 
>>>>>>>> Ethan
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> On Dec 7, 2010, at 1:30 PM, Jed Brown wrote:
>>>>>>>>> 
>>>>>>>>>> On Tue, Dec 7, 2010 at 20:21, Ethan Coon <ecoon at lanl.gov> wrote:
>>>>>>>>>> 'd like a DA where there are ghost cells on every boundary, and some of
>>>>>>>>>> those ghost cells (but not all) are filled in with periodic values.
>>>>>>>>>> 
>>>>>>>>>> It would be useful to people doing explicit stuff if there was a way to get ghost nodes in the local vector without implying periodic communication (and weird coordinate management).
>>>>>>>>>> 
>>>>>>>>>> A related issue for purely explicit is to have a way to VecAXPY without needing to copy to and from a global vector.  (TSSSP has low-memory schemes, paying for an extra vector or two is actually significant in that context, and (less significant) I'm certain I can cook up a realistic benchmark where the memcpy costs more than 10%.)  I think I know how to implement this sharing transparently (more-or-less using VecNest) so we could make it non-default but be able to activate it at runtime.
>>>>>>>>> 
>>>>>>>>> Why can you not use VecAXPY() on the local Vecs?
>>>>>>>>> 
>>>>>>>>> Barry
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> Jed
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>>> -- 
>>>>>>> ------------------------------------
>>>>>>> Ethan Coon
>>>>>>> Post-Doctoral Researcher
>>>>>>> Applied Mathematics - T-5
>>>>>>> Los Alamos National Laboratory
>>>>>>> 505-665-8289
>>>>>>> 
>>>>>>> http://www.ldeo.columbia.edu/~ecoon/
>>>>>>> ------------------------------------
>>>>>>> 
>>>>>> 
>>>>> 
>>>>> -- 
>>>>> ------------------------------------
>>>>> Ethan Coon
>>>>> Post-Doctoral Researcher
>>>>> Applied Mathematics - T-5
>>>>> Los Alamos National Laboratory
>>>>> 505-665-8289
>>>>> 
>>>>> http://www.ldeo.columbia.edu/~ecoon/
>>>>> ------------------------------------
>>>>> 
>>>> 
>>> 
>>> -- 
>>> ------------------------------------
>>> Ethan Coon
>>> Post-Doctoral Researcher
>>> Applied Mathematics - T-5
>>> Los Alamos National Laboratory
>>> 505-665-8289
>>> 
>>> http://www.ldeo.columbia.edu/~ecoon/
>>> ------------------------------------
>>> 
>> 
> 
> -- 
> ------------------------------------
> Ethan Coon
> Post-Doctoral Researcher
> Applied Mathematics - T-5
> Los Alamos National Laboratory
> 505-665-8289
> 
> http://www.ldeo.columbia.edu/~ecoon/
> ------------------------------------
> <dmda-periodicity.txt>

More information about the petsc-dev mailing list