[petsc-dev] Patches like this make me want to cry

[Jed Brown]

On Thu, Jun 28, 2012 at 2:41 PM, Paul Mullowney <paulm at txcorp.com> wrote:

>  I moved all CUSPARSE functions into seqcusparse/aijcusparse.cu (and a
> similar mpi version). I was asked to do this some months ago. I did my
> best. I think it is significantly improved from before.
>
> I was also asked by multiple people to attempt to get Complex GPU
> capabilities working. I succeeded by making changes to petscmath.h. As Jed
> pointed out, some of the changes may have not been C99 compliant.
>

It needs to work with C89, not just C99. Global changes to handle complex
should also be done in separate patches.

I hate the idea that CUSP leaks out into user code so I would recommend
casting internally rather than changing it globally. That should also
enable use of complex without requiring the user to adopt a C++ compiler
for their own code (C99 _Complex is binary-compatible with std::complex and
cusp::complex).


>
> There is one commented out function in aijcusparse.cu. That can easily be
> fixed.
>
> Many of the other changes (especially to .cu) files were made to protect
> against builds breaking because many of the CUSP preconditioners do not
> support complex arithmetic.
>

Those should absolutely be in separate patches. Lumping it all into one
makes it impossible to review or test.


>
> I have asked for input from multiple people on several occasions and not
> gotten responses.
>
> What am I to do if I ask for input/feedback and don't get it?????
>

Next time, please provide the patch series. You could use "hg email" or
post your branch to bitbucket or elsewhere so that we can comment.

Also, please follow the coding guidelines. There is a developer's manual
and frequent discussion on petsc-dev about conventions. (If you notice
something that is adopted, but not in the developer's manual, please point
it out on the mailing list or make a patch.)


>
>
> -Paul
>
>
>
>
>  There are essentially no comments on this massive push. I was just
> reading it, getting mad. Jed mailed first.
> Pushes need enough comments that I can see exactly what functionality is
> being added and how it works.
> You will not be the maintainer of this functionality, we will. If we can't
> do it, it will be thrown out, and all your
> time will be wasted.
>
>     Matt
>
>
>>
>>>  This is non-portable:
>>>>
>>>> http://petsc.cs.iit.edu/petsc/petsc-dev/rev/66ca8db0d5f8
>>>>
>>>>
>>>> Can we please institute some sort of policy on patch
>>>> quality/reviewability? This one patch is going to take a significant amount
>>>> of fix-up (not made easier by the several merges since) and/or generate
>>>> several build failures and user inconvenience (petsc-maints). No doubt the
>>>> functionality is important, but we just don't have time to fix these things
>>>> line-by-line after they are pushed.
>>>>
>>>
>>>
>>
>
>
>  --
> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20120628/e2393998/attachment.html>