Pull requests

#81 Merged
Repository
Branch
madams/sor-opt
Repository
Branch
master

optimized SOR

Author
  1. Mark Adams
Reviewers
Description
No description
  • Learn about pull requests

Comments (7)

  1. Jed Brown

    Mark, can you put yourself in the shoes of someone with less context, either yourself next year or the rest of us trying to figure out how and why the code get the way it is.

    $ git log --oneline master..madams/sor-opt 
    3d38f94 all working
    98ecc339 all working
    a9293ac added all sizes in forward sweep
    5e846cf working with forward sweep split
    de0acf9 getting back to good innode.c
    aacab20 ...
    47bb0e1 removing debugs
    928e65f debug
    c2d9332 small change to ex54
    1092a6a fixed timers
    f9b6998 1st optimization
    

    We can hardly tell anything from this summary. Look at the code to find out the most basic information is not efficient. Could you please write commit messages that are useful to the reader? (Compare to typical commit messages in PETSc over the past few months.)

  2. Mark Adams author

    I guess I can remove these comments in some way and add an uber comment. How would I do that? I like to use a repo for interim commits. Am I missing a correct workflow here?

    1. Jed Brown

      You can use git rebase -i and use "reword" and "squash" to combine and reword commit messages. After that, you'll have to force push the branch and then update the pull request.

        1. Jed Brown

          You squashed this together, but then merged back:

          *   1a38143 (madams/sor-opt) Merge branch 'madams/sor-opt' of bitbucket.org:petsc/petsc into madams/sor-opt
          |\  
          | * 891f4ed removed C++ comments
          | * ab0bb76 removed makefile bug
          | * 3d38f94 all working
          | * 98ecc339 all working
          | * a9293ac added all sizes in forward sweep
          | * 5e846cf working with forward sweep split
          | * de0acf9 getting back to good innode.c
          | * aacab20 ...
          | * 47bb0e1 removing debugs
          | * 928e65f debug
          | * c2d9332 small change to ex54
          | * 1092a6a fixed timers
          | * f9b6998 1st optimization
          * | 534a7fe 1st optimization
          |/  
          *   cdb4a13 Merge branch 'jed/snes-linesearch-warning'
          

          rather than replacing the original, which could have been done using:

          $ git push origin +madams/sor-opt
          

          The other issue is that the new commit message "1st optimization" doesn't provide any explanation of what was done or why.

          I fixed the whitespace and formatting errors, then split the commit into three parts, and attempted to write useful commit messages.

          The first commit contains the scalar AIJ code which I think is fine and can be merged. The second has the Inode optimizations, but I'm not wild about the manual unrolling that you're using. What evidence do you have that the unrolled code is faster? It is definitely harder to read and probably makes the object code larger. If it's break-even or worse performance (possible), then it shouldn't be merged. The third commit adds the KSPSetUp to the example. That should be unnecessary for correctness, so why was it added?

          Finally, you may want to change your commit email to LBL:

          $ git config --global user.email mfadams@lbl.gov
          
          1. Mark Adams author

            The existing inode routines are unrolled an i just cloned them. The last two tests that I sent out used I inodes. A 2D and a 3D problem. I'm confused, you seem to think I added unrolling but you can see that I just modified and cloned the existing code.

            And the addition of KSPSetup to the test was to get timing for the solve more easily. This is such a trivail change to a test code that I did not think worth going through all of this formalism. Take it out if you wish.

            1. Jed Brown

              Sorry, I misread the diff and thought you had added the unrolling along columns. I'm still skeptical of its benefit, but it's clearly not a new thing.

              I thought the separate call to KSPSetUp was probably for profiling reasons, but I wonder if we should move it outside of the KSPSolve event. I don't like that the profile attributes time differently if the user calls KSPSetUp themselves or lets KSPSolve do it.

              I'll reword the messages and merge.