Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Types #43

Merged
merged 9 commits into from
May 20, 2017
Merged

Types #43

merged 9 commits into from
May 20, 2017

Conversation

cortner
Copy link
Contributor

@cortner cortner commented May 8, 2017

I suggest to not merge this yet. This is just a proof-of-concept for backtracking! only. Please have a quick look - if you are happy then I'll try to do the same for the remaining line searches.

Notes:

  • This PR will give - for the user - the functionality of using types instead of functions for LS, but internally very little has changed. Both of you (@pkofod and @anriseth) are probably better placed to carry out the internal changes. (though time permitting I'm obviously happy to do this too)
  • While I was at it, I also made a small addition to backtracking! adding a maxstep parameter; this requires that alpha * vecnorm(s, inf) <= maxstep. (the choice of norm is a little restrictive, but I'll do another PR later when this is done to generalise to arbitrary step-length constraints).

Please, somebody double-check that I've deprecated bt2! and bt3! correctly.

@codecov
Copy link

codecov bot commented May 8, 2017

Codecov Report

Merging #43 into master will increase coverage by 1.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #43     +/-   ##
=========================================
+ Coverage   47.52%   48.63%   +1.1%     
=========================================
  Files           7        8      +1     
  Lines         566      584     +18     
=========================================
+ Hits          269      284     +15     
- Misses        297      300      +3
Impacted Files Coverage Δ
src/strongwolfe.jl 75.32% <100%> (-5.76%) ⬇️
src/deprecate.jl 100% <100%> (ø)
src/hagerzhang.jl 33.01% <100%> (+0.47%) ⬆️
src/types.jl 58.33% <100%> (ø) ⬆️
src/basic.jl 100% <100%> (ø) ⬆️
src/morethuente.jl 53.97% <100%> (+0.26%) ⬆️
src/backtracking.jl 75% <100%> (+2.27%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fb4d38...c5256da. Read the comment docs.

@anriseth
Copy link
Collaborator

This looks good to me. Seems like it is quite straightforward to introduce types :)

I think that, while we're at it - we could evaluate whether the gr_scratch parameter is necessary. value_gradient! and gradient! makes it obsolote, as far as I can remember (was looking at this a few months ago)

@cortner
Copy link
Contributor Author

cortner commented May 16, 2017

Ok - will try to finish this over the course if the next few days

@cortner
Copy link
Contributor Author

cortner commented May 17, 2017

This should in principle complete the PR. Some more notes:

I've called the basic line search Static rather than Basic, this seems more descriptive to me.

At this point it would be good to decide whether the call trick is actually sufficient, or whether - in a second step - it would be better to replace it, i.e.,

(ls::LineSearch)(args...)

vs

linesearch(ls, args...) 

Either way, the next step - maybe still within this PR - might be to deprecate all of the line search functions morethuente!, hagerzhang!, etc and replace them with private functions _morethuente!, etc. (On that note: how do I create a custom deprecation warning?)

@pkofod
Copy link
Member

pkofod commented May 17, 2017

I've called the basic line search Static rather than Basic, this seems more descriptive to me.

I agree; basic could just as well be backtracking to some people

At this point it would be good to decide whether the call trick is actually sufficient, or whether - in a second step - it would be better to replace it, i.e.,

(ls::LineSearch)(args...)

vs

linesearch(ls, args...)

why would the second be preferred?

Either way, the next step - maybe still within this PR - might be to deprecate all of the line search functions morethuente!, hagerzhang!, etc and replace them with private functions _morethuente!, etc. (On that note: how do I create a custom deprecation warning?)

yes, this needs to be done, and I think the "privatization" is a good idea. Let me get back to you on the "custom" warning unless someone gets to it before me (I need to go)

@pkofod
Copy link
Member

pkofod commented May 17, 2017

regarding the custom dep, you can do something like

const has_deprecated_xyz = Ref(false)

function xyz(args)
        if !has_deprecated_xyz![]
            warn("xyz is deprecated, please ... descriptive message here ...")
            has_deprecated_xyz![] = true
        end
    ... code here ...
    return ...
end

@cortner
Copy link
Contributor Author

cortner commented May 17, 2017

ok so pretty much hand-code it, that will be fine.

@cortner
Copy link
Contributor Author

cortner commented May 17, 2017

(ls::LineSearch)(args...) vs linesearch(ls, args...)

why would the second be preferred?

The second seems the more natural construction in the dispatch context, but it really doesn't have to be. I am perfectly happy with making linesearch types callable. In particular the first type requires far no changes throughout Optim and other codes that might be using LineSearches, so unlesss there is a more compelling reason than "it feels more natural" I think it is the right way to go.

@cortner
Copy link
Contributor Author

cortner commented May 17, 2017

const has_deprecated_xyz = Ref(false)

why Ref?

@cortner
Copy link
Contributor Author

cortner commented May 17, 2017

see the PR #411 in Optim for the corresponding changes in Optim.

@anriseth
Copy link
Collaborator

(ls::LineSearch)(args...) vs linesearch(ls, args...)

why would the second be preferred?

The dispatch approach would align with Optim's optimize approach. So maybe a preference due to consistency?
I haven't tried using callable types much, so I don't know how they work and whether there are any pros and cons vs dispatch. It would be interesting to hear if anyone knows:)

@anriseth
Copy link
Collaborator

Ready to merge?

const has_deprecated_xyz = Ref(false)
why Ref?

I'm curious as well

@cortner
Copy link
Contributor Author

cortner commented May 20, 2017

hm - when I was writing it, it started to make sense, I'll need to look at the code again.

@cortner
Copy link
Contributor Author

cortner commented May 20, 2017

I remember now - this is actually the only way to do it - at least as I've implemented the deprecation in the end. If you are more comfortable with macros than I am then you could probably rewrite these deprecations much more elegantly though.

P.S.: if you read the code it ought to be clear, but let me know if I need to clarify anything

@anriseth
Copy link
Collaborator

I see. No I'm not a big macro genius - just hadn't seen Ref before.

Is this ready to merge?

@cortner
Copy link
Contributor Author

cortner commented May 20, 2017

from my end, yes it is ready

@pkofod
Copy link
Member

pkofod commented May 20, 2017

hmm, can't really remember the technical details for using Ref... I got it from @KristofferC when I used it in Optim :)

@pkofod
Copy link
Member

pkofod commented May 20, 2017

The function approach would be fine by me. It's currently called perform_linesearch in Optim, but doesn't have to have the perform included for all I care.

@anriseth anriseth merged commit 9bfecd3 into JuliaNLSolvers:master May 20, 2017
@anriseth
Copy link
Collaborator

Great, thanks!

@pkofod
Copy link
Member

pkofod commented May 20, 2017

I think we should actually define the function (whatever name we choose) in NLSolversBase. This would mean that another linesearch package could just import NLSolversBase.linesearch, adhere to the same syntax, and then that would work out of the box in Optim.

@cortner
Copy link
Contributor Author

cortner commented May 20, 2017

Does it also become important then to define an abstract supertype AbstractLineSearch? I don't really see the necessity here, but maybe I am missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants