added and pushed.
satish
On Thu, 10 Mar 2011, Barry Smith wrote:
>
> 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>
>
>
