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