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>
