Skip to content

Conversation

@tlienart
Copy link
Contributor

@tlienart tlienart commented Aug 23, 2019

This is in view of JuliaNLSolvers/Optim.jl#738 @pkofod

  • add only_fg_and_hv for a constructor where only these two things are provided
  • add only_fghv for a constructor that looks like only_fgh and allows re-using computations for the construction of Hv (a common trick using the construction of the gradient to get Hv, see issue quoted above).

This may break Optim in that the call to fg is now the "standard" format used elsewhere IE: (f, g, x) instead of just (g, x); I'll open a PR for Optim soon too.

PS: failure on nightly are due to the project toml not being up to date, see PR #108

Edit marked as "do not merge" for now, the ambiguity of calling convention for fg either with (g, x) or (f, g, x) is causing me some trouble now ok, see also JuliaNLSolvers/Optim.jl#742

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #109 into master will increase coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #109      +/-   ##
=========================================
+ Coverage   93.46%   93.6%   +0.13%     
=========================================
  Files          10      10              
  Lines         490     500      +10     
=========================================
+ Hits          458     468      +10     
  Misses         32      32
Impacted Files Coverage Δ
src/NLSolversBase.jl 80% <ø> (ø) ⬆️
src/objective_types/twicedifferentiablehv.jl 100% <100%> (ø) ⬆️
src/objective_types/incomplete.jl 98% <100%> (+0.32%) ⬆️

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 6af840c...71c6403. Read the comment docs.

@tlienart tlienart changed the title 'only' constructors with TwiceDifferentiableHv [do not merge] 'only' constructors with TwiceDifferentiableHv Aug 23, 2019
@tlienart tlienart changed the title [do not merge] 'only' constructors with TwiceDifferentiableHv 'only' constructors with TwiceDifferentiableHv Aug 23, 2019
@pkofod
Copy link
Member

pkofod commented Sep 5, 2019

Welcome to the frightening world of NLSolversBase :) I will have a look as soon as possible. Thanks

@tlienart
Copy link
Contributor Author

bump?

@pkofod
Copy link
Member

pkofod commented Sep 28, 2019

Great, looks good! Sorry for the long wait.

@pkofod pkofod merged commit 3202633 into JuliaNLSolvers:master Sep 28, 2019
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.

2 participants