On Mar 10, 2011, at 2:57 PM, Lisandro Dalcin wrote:
> On 10 March 2011 17:30, Ethan Coon <ecoon at lanl.gov> 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.
>>
>
> What about renaming DMDAPeriodicType -> DMDABoundaryType ?
I like this suggestion. A patch to make this change can easily be generated.
Barry
> GHOSTED and
> PERIODIC are different concepts, the enumeration should have a more
> generic name.
>
> BTW, Why don't you?:
>
> +#define DMDAXPeriodic(pt) ((pt) & (DMDA_XPERIODIC ^ DMDA_XGHOSTED))
> instead of the hardwired value?
>
>
>> 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/
>> ------------------------------------
>>
>
>
>
> --
> Lisandro Dalcin
> ---------------
> CIMEC (INTEC/CONICET-UNL)
> Predio CONICET-Santa Fe
> Colectora RN 168 Km 472, Paraje El Pozo
> 3000 Santa Fe, Argentina
> Tel: +54-342-4511594 (ext 1011)
> Tel/Fax: +54-342-4511169
