Modifications and Extension of Preconditioning #191

cortner · 2016-04-21T10:20:01Z

This was discussed in Issue 188.

created precon.jl
added preconditioning to GradientDescent and to LBFGS
added test/precon.jl to test preconditioning capabilities
added a matrix preconditioner

Documentation has been added in a separate pull request #190 .

I think this is just a quick and dirty implementation. I suggest more
substantial changes in this direction in Issue 188.

…ent with dispatch functionality; implemented a rudimentary constant matrix preconditioner; all tests passing. TODO: preconditioned L-BFGS and benchmarks

pkofod · 2016-05-09T21:30:15Z

@timholy do you have time for a short glimpse at this?

pkofod · 2016-05-09T21:31:26Z

src/gradient_descent.jl

@@ -51,15 +56,15 @@ function optimize{T}(d::DifferentiableFunction,

    # The current search direction
    s = similar(x)
-
+    


I think these tabs were added by mistake? (three additional instances below)

now reverted.

pkofod · 2016-05-09T21:36:48Z

Thank you for the PR, and sorry for the delay. I've left some line notes, and pinged the original author of the code (as far as I can see).

timholy · 2016-05-09T22:09:49Z

Enough has changed that I don't have time to dig in now and offer a good review, but in general terms I'm totally in favor of taking any code I wrote and making it better.

cortner · 2016-05-10T07:53:57Z

From @timholy 's perspective the only change is the move of the preconditioner code to precon.jl.

cortner · 2016-05-10T07:58:21Z

I made the changes and pushed them - do I need to do anything to update the pull request?

pkofod · 2016-05-10T08:00:18Z

No, if you push it to the same exact branch, it should update. Are you sure you committed before pushing?

cortner · 2016-05-10T08:30:46Z

Yes, I am sure:
https://github.com/cortner/Optim.jl/commit/7f7987717bd807f0aa46ff47faa7c7ec6f44ae2d

I haven't done this before, but am following Julia documentation. Sorry, if I've messed something up. I can always create a new pull request.

pkofod · 2016-05-10T08:34:45Z

Ah, okay. The first time you pushed to your fork, you pushed to cortner:pull-request/5a64f9ae, but this time you pushed to cortner:precon. You need to git push REPO pull-request/5a64f9ae. Depending on what your repo is called locally (see git remote -v), you should replace REPO with that.

cortner · 2016-05-10T08:59:55Z

$ git remote -v
cortner https://github.com/cortner/Optim.jl (fetch)
cortner https://github.com/cortner/Optim.jl (push)
origin  git://github.com/JuliaOpt/Optim.jl.git (fetch)
origin  git@github.com:JuliaOpt/Optim.jl.git (push)

$ git push cortner pull-request/5a64f9ae
error: src refspec pull-request/5a64f9ae does not match any.
error: failed to push some refs to 'https://github.com/cortner/Optim.jl'

cortner · 2016-05-10T09:01:19Z

I'm really sorry for making a mess. Here is the Julia documentation, is this incorrect and should it be fixed?

The package owner may suggest additional improvements. To respond to those suggestions, you can easily update the pull request (this only works for changes that have not already been merged; for merged pull requests, make new changes by starting a new branch):

If you’ve changed branches in the meantime, make sure you go back to the same branch with git checkout fixbar (from shell mode) or Pkg.checkout("Foo", "fixbar") (from the Julia prompt).
As above, make your changes, run the tests, and commit your changes.
From the shell, type git push. This will add your new commit(s) to the same pull request; you should see them appear automatically on the page holding the discussion of your pull request.

codecov-io · 2016-05-10T09:02:50Z

Current coverage is 85.26%

Merging #191 into master will decrease coverage by -<.01%

@@             master       #191   diff @@
==========================================
  Files            30         31     +1   
  Lines          1866       1872     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1592       1596     +4   
- Misses          274        276     +2   
  Partials          0          0

3 files in src were modified. more
- Misses +1
- Hits -7

Powered by Codecov. Last updated by 1cf3fbe...f71bad8

cortner · 2016-05-10T09:03:09Z

Ok, this did it:
$ git push cortner precon:pull-request/5a64f9ae

pkofod · 2016-05-10T09:17:54Z

Yes, sorry, that was my bad! I thought the branch was also called pull-request/5a64f9ae locally on your computer!

pkofod · 2016-05-10T09:24:49Z

src/gradient_descent.jl

        end
-
+        


Seems like you missed one. Sorry, but this just makes it easier when going through old versions looking through changes.

I'm very happy to do it, since this is your code. But for what it's worth, I find it harder to read that way since the indentation guides don't show up correctly.

I'm not sure what you mean. I'm talking about line 97 which is an empty line. Is it something in your editor you are referring to?

Yes, I can add an image later

EMACS:

But the same problem does not exist on Atom.io, so just Emacs-related. I didn't realise that.

pkofod · 2016-05-10T09:26:52Z

Good to see it worked out :) One minor comment has been made. I'm kind of wondering why LBFGS needs the {T} and P::T's, maybe I'm missing something. However, I do realize that you just followed ConjugateGradient, so just ignore this last remark.

cortner · 2016-05-10T10:08:14Z

thank you for the help.

pkofod · 2016-05-10T13:07:30Z

Are you familiar with the process of squashing in git? It basically means to rewrite history such that it appears as if several commits were made at once.

I am asking because I would prefer if you could do this to your 6 commits. Again, this is beneficial if we need to go back in time through the git history, to find a bug introduced somewhere. Sometimes it would be okay to have more than one commit, for example: add functionality, add tests and documentation (although in many cases this should be one commit only). However, since some of the intermediate commits had "problems" (things that were fixed in later commits), it clutters the history, and makes bisects more difficult.

I would propose that you simply squash everything into one commit, that I can then merge into master.

The process is quite easy. I do recommend making a back-up branch in case you mess something up (I did the first few times...)

git checkout -b precon2
git rebase -i HEAD~6

This should show you an editor with all your commits. They will all have "pick" written at the beginning of their respective lines. Change the five last (bottom) to squash, and exit and save. This will show a new prompt where you have to change the commit messages. Simply delete the contents of the last five, and write something like Refactor preconditioning, and add preconditioning to LBFGS and GradientDescent or whatever you prefer. Then you can

git push -f cortner precon2:pull-request/5a64f9ae

Notice the last command is quite dangerous, as you force push (-f) to the branch pull-request/5a64f9ae which overwrites what is there now. That is why I recommended you to create the precon2 branch. Then you have the old precon branch locally if anything goes wrong.

KristofferC · 2016-05-10T13:11:00Z

FWIW you can squash from the github ui now. When you press merge you get to chose to merge it with a merge commit or to squash everything.

pkofod · 2016-05-10T13:13:17Z

FWIW you can squash from the github ui now. When you press merge you get to chose to merge it with a merge commit or to squash everything.

Oh, so I can do it? Then don't bother @cortner , unless you want to try it out.

KristofferC · 2016-05-10T13:13:44Z

Yes you can do it.

Squashed commits: [5a64f9a] added test/precon.jl [4066ed9] implemented precond for L-BFGS; reverted the precondprep implementation to passing it as argument (nor now); replaced precondprep with precondprep! and precondprepbox with precondprepbox! and precondfwd with precondfwd!; fixed constrained.jl; tests all pass [f7c92d0] further comments [e8a4cf1] added preconditioning to GradientDescent; implemented tests; all tests pass

cortner · 2016-05-10T13:26:55Z

this ok?

pkofod · 2016-05-10T14:15:28Z

Yes, Ill finish this when I'm at a computer. Thank you for the pr.

pkofod · 2016-05-10T17:59:29Z

Done.

Thanks for the PR. Hope you enjoyed the Github ride! Let us keep the preconditioning discussion open in #188 .

KristofferC · 2016-05-10T18:01:19Z

Nice contribution. Thanks.

moved preconditioning into precon.jl, replaced precondprep! argum…

50ddb01

…ent with dispatch functionality; implemented a rudimentary constant matrix preconditioner; all tests passing. TODO: preconditioned L-BFGS and benchmarks

pkofod reviewed May 9, 2016
View reviewed changes

pkofod reviewed May 10, 2016
View reviewed changes

pkofod merged commit 3ac0971 into JuliaNLSolvers:master May 10, 2016

cortner deleted the pull-request/5a64f9ae branch May 10, 2016 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifications and Extension of Preconditioning #191

Modifications and Extension of Preconditioning #191

cortner commented Apr 21, 2016 •

edited

Loading

pkofod commented May 9, 2016

pkofod May 9, 2016

cortner May 10, 2016

pkofod commented May 9, 2016

timholy commented May 9, 2016

cortner commented May 10, 2016

cortner commented May 10, 2016

pkofod commented May 10, 2016

cortner commented May 10, 2016 •

edited

Loading

pkofod commented May 10, 2016

cortner commented May 10, 2016

cortner commented May 10, 2016

codecov-io commented May 10, 2016 •

edited

Loading

cortner commented May 10, 2016

pkofod commented May 10, 2016

pkofod May 10, 2016

cortner May 10, 2016

pkofod May 10, 2016

cortner May 10, 2016 •

edited

Loading

pkofod commented May 10, 2016

cortner commented May 10, 2016

pkofod commented May 10, 2016 •

edited

Loading

KristofferC commented May 10, 2016

pkofod commented May 10, 2016

KristofferC commented May 10, 2016

cortner commented May 10, 2016

pkofod commented May 10, 2016

pkofod commented May 10, 2016 •

edited

Loading

KristofferC commented May 10, 2016

		@@ -51,15 +56,15 @@ function optimize{T}(d::DifferentiableFunction,

		# The current search direction
		s = similar(x)

Modifications and Extension of Preconditioning #191

Modifications and Extension of Preconditioning #191

Conversation

cortner commented Apr 21, 2016 • edited Loading

pkofod commented May 9, 2016

pkofod May 9, 2016

Choose a reason for hiding this comment

cortner May 10, 2016

Choose a reason for hiding this comment

pkofod commented May 9, 2016

timholy commented May 9, 2016

cortner commented May 10, 2016

cortner commented May 10, 2016

pkofod commented May 10, 2016

cortner commented May 10, 2016 • edited Loading

pkofod commented May 10, 2016

cortner commented May 10, 2016

cortner commented May 10, 2016

codecov-io commented May 10, 2016 • edited Loading

Current coverage is 85.26%

cortner commented May 10, 2016

pkofod commented May 10, 2016

pkofod May 10, 2016

Choose a reason for hiding this comment

cortner May 10, 2016

Choose a reason for hiding this comment

pkofod May 10, 2016

Choose a reason for hiding this comment

cortner May 10, 2016 • edited Loading

Choose a reason for hiding this comment

pkofod commented May 10, 2016

cortner commented May 10, 2016

pkofod commented May 10, 2016 • edited Loading

KristofferC commented May 10, 2016

pkofod commented May 10, 2016

KristofferC commented May 10, 2016

cortner commented May 10, 2016

pkofod commented May 10, 2016

pkofod commented May 10, 2016 • edited Loading

KristofferC commented May 10, 2016

cortner commented Apr 21, 2016 •

edited

Loading

cortner commented May 10, 2016 •

edited

Loading

codecov-io commented May 10, 2016 •

edited

Loading

cortner May 10, 2016 •

edited

Loading

pkofod commented May 10, 2016 •

edited

Loading

pkofod commented May 10, 2016 •

edited

Loading