February 2013 - petsc-dev - mailman3.cels.anl.gov

Re: [petsc-dev] I hate this one
by Milad Fatenejad 13 Feb '13

13 Feb '13

Hello: I am working on modifying PETSc to interoperate with the MOAB mesh library. I'd love to get some of Karl's scripts to ensure that my code is compliant with the style guide. Are they available somewhere? Thank You Milad On Mon, Jan 21, 2013 at 10:19 AM, Sean Farley <sean(a)mcs.anl.gov> wrote: > On Mon, Jan 21, 2013 at 10:06 AM, Karl Rupp <rupp(a)mcs.anl.gov> wrote: > > Hi Matt, > > > > > >> Again, there are limits to everything, and this surpasses the useful > >> > >> limit to this kind of specification. This is not personal > >> expression, this > >> is ease of reading. > >> > >> > >> Also, judging by the ENORMOUS number of source code changes, "everyone" > >> was not following the informal guidelines. > > > > > > Yes, the changes are 'enormous' if you count the absolute number of > lines. > > However, my overall impression while refactoring was that I touched ~1% > of > > the total code base only. It's the huge size of the code base that makes > the > > number of changes appear to be high. > > > > Also, an estimated 50% of the violations stem from a copy&paste of older > > code or previous examples rather than intentionally ignoring the style > > guidelines. Thus, by bringing everything to a consistent state, a (large) > > number of such copy&paste-induced violations are no longer going to > happen > > at all. > > > > The PETSc coding style guide needs to define exactly one style. Your > style > > preference obviously differs. My personal preference differs slightly, > too. > > Maybe Jed and/or others would prefer another slight variation. > Nevertheless, > > allowing a bit more flexibility in one's own personal coding style by > > following one common coding style is to the benefit of the whole project. > > Visual consistency is an obvious one, but most importantly it aids in > > setting up other scripts for increased uniformity among the different > PETSc > > modules. > > > > You may argue that it does not matter whether we have 'for(...)' or 'for > > (...)'. While this is certainly right for a compiler, I would like to > give > > you one example where it *does* make a difference: > > src/mat/interface/matrix.c: > > PetscErrorCode MatSetValuesAdifor(Mat mat,PetscInt nl,void *v) > > So, keeping consistency with the style guide, we would write > > for (...) {...} > > MatSetValuesAdifor(...) {...} > > and there is the additional blank contributing to the difference between > the > > function call and the for-loop. If, however, we have > > for(...) {...} > > MatSetValuesAdifor(...) {...} > > any scripts additionally need to check for characters in front of 'for'. > > Fortunately, it is *currently* sufficient to just consider all characters > > other than a blank in front of 'for' to be a function call. However, one > day > > we might find code such as > > do_something(...);for(...) {...}; > > or even > > cilk_for(...) > > and all of a sudden we are unable to use any simple checker scripts any > > more. With a common PETSc style, the additional blank would still allow > for > > far simpler checks and refactoring if required. > > > > Just my 2 cents… > > Thanks, Karl. I appreciate your effort on this front. >

1 0

Re: [petsc-dev] Fix for mataijcusp.cu
by Karl Rupp 13 Feb '13

13 Feb '13

Hi Jose, > By the way, I am getting a warning which I think is new - when compiling with nvcc in optimized builds: > > /home/jroman/soft/petsc-dev/include/petsc-private/petscimpl.h(201): warning: "cc" clobber ignored I've also seen this in optimized builds using a Valgrind header. I don't know a fix for it, though, as this is potentially something introduced from the outside/nvcc. Best regards, Karli

1 0

Re: [petsc-dev] questions on new include organization
by Jed Brown 13 Feb '13

13 Feb '13

On Wed, Feb 13, 2013 at 8:06 AM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > You misunderstand me. I am not advocating replacing the more efficient > access code using offset plus length to ALWAYS calling ISGetIndices() and > using the indices to do the access. I absolutely am not advocating that. > Recall we sometimes have subclasses that do not implement (or get used) all > the methods of the base class. > > What I am advocating is recognizing that PetscSection and IS have (in > the abstract) very similar jobs and thus organizing the source code > (naming, possible methods etc) to reflect this. Rather than having > seemingly two completely unrelated classes. > > For example, and I'm not advocating this naming, we could have a base > abstract class of PetscIndex with PetscIndexIS and PetscIndexSection, now > people always see the connection. The weakness of this exact approach is > though both things are related to indexing, it is not clear they have any > common methods (as you both have pointed out) so "deriving" them both from > PetscIndex might not be best. > Hmm, PetscSection is strictly more general, so it would be possible for IS to be an implementation of PetscSection. Since these things are (at least after setup) immutable, the hierarchy doesn't come crashing down. The problem is that 1. With the way we do inheritance, you wouldn't be able to statically determine at the call site whether you had a general Section or the more restrictive IS. This affects ability to reason about the code locally, and in _most_ locations, we really need an IS. 2. PetscSection (at least) is performance-critical due to fine-grained accessors, so dynamic dispatch is a problem. If we had PetscSections of homogenous type, we could relieve it with multi-accessors, but that would be a weird interface by PETSc standards.

1 0

Re: [petsc-dev] questions on new include organization
by Matthew Knepley 13 Feb '13

13 Feb '13

On Wed, Feb 13, 2013 at 9:07 AM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > > On Feb 13, 2013, at 7:50 AM, Jed Brown <jedbrown(a)mcs.anl.gov> wrote: > > > > > On Wed, Feb 13, 2013 at 7:40 AM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > > Regardless of exactly how this shakes out I think you both have to > agree that PetscSection is a bit of an oddball and it should be more > "integrated" with the "IS stuff" in that we have a single source code > location (directory) and set of concepts related to indexing things. And > don't have some in the Vec directory. > > So, for now, I won't change names or functionality but would like > permission to move source around. Who knows, maybe in the end the is > directory will get a more suitable name. > > > > That's fine with me, but note that vsection.c depends on Vec, but IS > does not depend on Vec. vsection.c depends on Vec so it can't simply be > moved to src/vec/is. > > Understood. I already noted in my initial email that it would be split. So we put the other piece in vec/utils? Matt > > Barry > > > > > > > Barry > > > > As you know I really really like having names that convey connections > left to right, KSPGMRES, PC_ILU etc. I think this helps make the learning > and understanding curve lower. Now people see IS and PetscSection and they > are two completely unrelated things to their eyes but in fact they are not > unrelated and I would like to convey that somehow in the future. > > > > BTW: I consider it a terrible tragedy that in (for example C++ and Java) > one can define a subclass of a class and just use a completely arbitrary > ASCII name for the subclass completely unrelated to the class it is derived > from, talk about losing information. > > > > Just be glad not too many projects chose the German way, using a > three-term recurrence to compute the designation order, and always > withholding the verb until the end. ;-) > > -- What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead. -- Norbert Wiener

1 0

Re: [petsc-dev] Fix for mataijcusp.cu
by Jose E. Roman 13 Feb '13

13 Feb '13

El 13/02/2013, a las 15:17, Barry Smith escribió: > > Thanks for noting this. Is the problem the (*right)->map operation in? > > ierr = PetscLayoutReference(mat->cmap,&(*right)->map);CHKERRQ(ierr); > > This can be corrected easily by replacing the PetscLayoutReference() here with VecSetLayout(). Does that resolve the problem? > > General note: We should really move the PetscLayout operations out of Vec mpi impl location. > > Barry > Yes it works. https://bitbucket.org/petsc/petsc-dev/commits/8264b0c102238be7a957e713db98c… By the way, I am getting a warning which I think is new - when compiling with nvcc in optimized builds: /home/jroman/soft/petsc-dev/include/petsc-private/petscimpl.h(201): warning: "cc" clobber ignored Jose

1 0

Re: [petsc-dev] Fix for mataijcusp.cu
by Barry Smith 13 Feb '13

13 Feb '13

Thanks for noting this. Is the problem the (*right)->map operation in? ierr = PetscLayoutReference(mat->cmap,&(*right)->map);CHKERRQ(ierr); This can be corrected easily by replacing the PetscLayoutReference() here with VecSetLayout(). Does that resolve the problem? General note: We should really move the PetscLayout operations out of Vec mpi impl location. Barry On Feb 13, 2013, at 3:51 AM, "Jose E. Roman" <jroman(a)dsic.upv.es> wrote: > mataijcusp.cu does not currently compile (MatGetVecs_MPIAIJCUSP accesses private Vec members). > Here is a fix. I did not push because maybe you prefer to use VecGetLayout here. > Jose > > > --- a/src/mat/impls/aij/mpi/mpicusp/mpiaijcusp.cu Tue Feb 12 23:21:30 2013 -0600 > +++ b/src/mat/impls/aij/mpi/mpicusp/mpiaijcusp.cu Wed Feb 13 10:42:59 2013 +0100 > @@ -3,6 +3,7 @@ > #include <../src/mat/impls/aij/mpi/mpiaij.h> /*I "petscmat.h" I*/ > PETSC_CUDA_EXTERN_C_END > #include "mpicuspmatimpl.h" > +#include <petsc-private/vecimpl.h> > > #undef __FUNCT__ > #define __FUNCT__ "MatMPIAIJSetPreallocation_MPIAIJCUSP" >

1 0

Re: [petsc-dev] questions on new include organization
by Barry Smith 13 Feb '13

13 Feb '13

On Feb 13, 2013, at 7:50 AM, Jed Brown <jedbrown(a)mcs.anl.gov> wrote: > > On Wed, Feb 13, 2013 at 7:40 AM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > Regardless of exactly how this shakes out I think you both have to agree that PetscSection is a bit of an oddball and it should be more "integrated" with the "IS stuff" in that we have a single source code location (directory) and set of concepts related to indexing things. And don't have some in the Vec directory. > So, for now, I won't change names or functionality but would like permission to move source around. Who knows, maybe in the end the is directory will get a more suitable name. > > That's fine with me, but note that vsection.c depends on Vec, but IS does not depend on Vec. vsection.c depends on Vec so it can't simply be moved to src/vec/is. Understood. I already noted in my initial email that it would be split. Barry > > > Barry > > As you know I really really like having names that convey connections left to right, KSPGMRES, PC_ILU etc. I think this helps make the learning and understanding curve lower. Now people see IS and PetscSection and they are two completely unrelated things to their eyes but in fact they are not unrelated and I would like to convey that somehow in the future. > > BTW: I consider it a terrible tragedy that in (for example C++ and Java) one can define a subclass of a class and just use a completely arbitrary ASCII name for the subclass completely unrelated to the class it is derived from, talk about losing information. > > Just be glad not too many projects chose the German way, using a three-term recurrence to compute the designation order, and always withholding the verb until the end. ;-)

1 0

Re: [petsc-dev] questions on new include organization
by Barry Smith 13 Feb '13

13 Feb '13

On Feb 13, 2013, at 7:41 AM, Jed Brown <jedbrown(a)mcs.anl.gov> wrote: > > On Wed, Feb 13, 2013 at 7:31 AM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > > 1) ISGetIndices(is,int group_number, &indices) gives the indices for a group or > > > > So by convention, group 0 is "offset" and group 1 is "size"? > > No, group_number is the number of the clump. and indices are all the values in that clump > > Then you need to also access the size of the clump, but since the primary purpose is for indexing into another array, you have to copy out a buffer (thus requiring a matching Restore). When using (int) -> (offset, size), we get direct access. It makes this inner loop code a lot simpler and faster. Consider DMPlexPointLocalRef(), which is effectively You misunderstand me. I am not advocating replacing the more efficient access code using offset plus length to ALWAYS calling ISGetIndices() and using the indices to do the access. I absolutely am not advocating that. Recall we sometimes have subclasses that do not implement (or get used) all the methods of the base class. What I am advocating is recognizing that PetscSection and IS have (in the abstract) very similar jobs and thus organizing the source code (naming, possible methods etc) to reflect this. Rather than having seemingly two completely unrelated classes. For example, and I'm not advocating this naming, we could have a base abstract class of PetscIndex with PetscIndexIS and PetscIndexSection, now people always see the connection. The weakness of this exact approach is though both things are related to indexing, it is not clear they have any common methods (as you both have pointed out) so "deriving" them both from PetscIndex might not be best. Barry > > ierr = PetscSectionGetOffset(dm->defaultGlobalSection,point,&offset);CHKERRQ(ierr); > *(PetscScalar**)ptr = (start >= 0) ? array + start - dm->map->rstart : NULL; > > This gives read+write access to that piece of the array. If PetscSectionGetOffset instead returned discontiguous indices, we'd need a work buffer and copy-back semantics, or we'd need to push that unstructured indexing burden onto the user.

1 0

Re: [petsc-dev] questions on new include organization
by Matthew Knepley 13 Feb '13

13 Feb '13

On Wed, Feb 13, 2013 at 8:40 AM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > > Regardless of exactly how this shakes out I think you both have to agree > that PetscSection is a bit of an oddball and it should be more "integrated" > with the "IS stuff" in that we have a single source code location > (directory) and set of concepts related to indexing things. And don't have > some in the Vec directory. > > So, for now, I won't change names or functionality but would like > permission to move source around. Who knows, maybe in the end the is > directory will get a more suitable name. > Yes, I completely agree with the move. There is really only one file right now. Matt > Barry > > As you know I really really like having names that convey connections left > to right, KSPGMRES, PC_ILU etc. I think this helps make the learning and > understanding curve lower. Now people see IS and PetscSection and they are > two completely unrelated things to their eyes but in fact they are not > unrelated and I would like to convey that somehow in the future. > > BTW: I consider it a terrible tragedy that in (for example C++ and Java) > one can define a subclass of a class and just use a completely arbitrary > ASCII name for the subclass completely unrelated to the class it is derived > from, talk about losing information. > > > On Feb 13, 2013, at 6:40 AM, Matthew Knepley <knepley(a)gmail.com> wrote: > > > On Wed, Feb 13, 2013 at 12:45 AM, Jed Brown <jedbrown(a)mcs.anl.gov> > wrote: > > > > On Tue, Feb 12, 2013 at 11:34 PM, Barry Smith <bsmith(a)mcs.anl.gov> > wrote: > > Is it? Maybe a more general definition of PetscSection would be better > as map from (int) -> IS Matt's current usage is very restricted to only > contiguous blocks of elements in a group. Need that be it? Should that be > it? > > > > There is some hackery to ascribe "field" structure to the output of the > map, but I'm skeptical of whether it belongs at this level. > > > > The granularity is normally very small so we certainly don't want to pay > the memory or performance overhead of an IS for each output. Although I'm > sure we can think up scenarios where that flexibility would be useful, I > think generality in the outputs here would add significant complexity with > limited practical return. > > > > Note that PetscSection is currently accessed using function calls in > inner loops. This already accounts for a noticeable amount of time for a > CFD application. So we either need a coarser grained access API or we need > a non-polymorphic accessor for use in hot access sites. (This is a rare > case where indirect function call overhead matters.) > > > > I think its helpful to look at the one universal use case right now: > > > > You would like to describe a function over a mesh, so > > associate functions with each part of the mesh and > > store coefficients of these functions. > > This happens for: > > mesh geometry > > PDE solutions and auxiliary fields (like viscosity) > > the mesh itself (the adjacency is a field over the points) > > > > The inner loops Jed is referring to are structured like this > > > > VecGetArray(sol, &solArray); > > Loop over pieces (like cell, face, etc.) > > Get solution on this piece: > > PetscSectionGetDof(solSection, piece, &dof), > PetscSectionGetOffset(solSection, piece, &off) > > pieceArray = &solArray[off]; > > Jed condenses this to DMPlexPointLocalRead(solDM, piece, solArray, > &pieceArray) > > > > Thus this adds structure to the Vec sol by breaking it into irregular > pieces. The lower level > > function Dof/Offset() are definitely necessary so you can reason about > the operations. I think > > you could get away with only the array offsetting being fast, but I > don't see how you make that > > fast and those slow. > > > > Barry: > Is it? Maybe a more general definition of PetscSection would be > better as map from (int) -> IS Matt's current usage is very restricted to > only contiguous blocks of elements in a group. Need that be it? Should that > be it? > > > > I think you are missing the point here. I use this all over the place. > This is PetscSection+IS. You put your > > non-contiguous indices in the IS and index into it using PetscSection. > PetscSection is the composable part > > of this indexing-by-groups. It can be combined with IS or Vec or > anything. I think it is the right building block. > > > > Matt > > -- > > What most experimenters take for granted before they begin their > experiments is infinitely more interesting than any results to which their > experiments lead. > > -- Norbert Wiener > > -- What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead. -- Norbert Wiener

1 0

Re: [petsc-dev] questions on new include organization
by Jed Brown 13 Feb '13

13 Feb '13

On Wed, Feb 13, 2013 at 7:40 AM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > Regardless of exactly how this shakes out I think you both have to agree > that PetscSection is a bit of an oddball and it should be more "integrated" > with the "IS stuff" in that we have a single source code location > (directory) and set of concepts related to indexing things. And don't have > some in the Vec directory. > So, for now, I won't change names or functionality but would like > permission to move source around. Who knows, maybe in the end the is > directory will get a more suitable name. > That's fine with me, but note that vsection.c depends on Vec, but IS does not depend on Vec. vsection.c depends on Vec so it can't simply be moved to src/vec/is. > > Barry > > As you know I really really like having names that convey connections left > to right, KSPGMRES, PC_ILU etc. I think this helps make the learning and > understanding curve lower. Now people see IS and PetscSection and they are > two completely unrelated things to their eyes but in fact they are not > unrelated and I would like to convey that somehow in the future. > > BTW: I consider it a terrible tragedy that in (for example C++ and Java) > one can define a subclass of a class and just use a completely arbitrary > ASCII name for the subclass completely unrelated to the class it is derived > from, talk about losing information. > Just be glad not too many projects chose the German way, using a three-term recurrence to compute the designation order, and always withholding the verb until the end. ;-)

1 0