Skip to content
This repository was archived by the owner on Nov 23, 2018. It is now read-only.

Conversation

@vladimir-ch
Copy link
Member

  • Implemented the other initial step size strategy described in Nocedal&Wright
  • In some CG implementations the formula is attributed to Shanno&Phua, but for consistence with QuadraticStepSize the type's name FirstOrderStepSize rather refers to how the step size is estimated. ShannoPhuaStepSize is an alternative name anyway.
  • This strategy seems to generate step size that work better with CG than those generated by QuadraticStepSize
  • Improve QuadraticStepSize #16 should be reviewed and merged first

@vladimir-ch vladimir-ch mentioned this pull request Dec 17, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling b0aca98 on first-order-step-size into 13d6e43 on master.

stepsizers.go Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a high-level comment about what it does.

@vladimir-ch vladimir-ch force-pushed the first-order-step-size branch from b0aca98 to bd49a15 Compare January 5, 2015 05:38
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.24%) when pulling bd49a15 on first-order-step-size into a1b9141 on master.

@vladimir-ch
Copy link
Member Author

PTAL @btracey

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.24%) when pulling d08d506 on first-order-step-size into a1b9141 on master.

@vladimir-ch vladimir-ch force-pushed the first-order-step-size branch from d08d506 to b73576a Compare January 7, 2015 06:05
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.26%) when pulling b73576a on first-order-step-size into 022ec44 on master.

stepsizers.go Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is backward.
... estimated at |g|_inf / InitialStepFactor

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, we want to take a small step when the gradient is large (when it is small, the step size is bound by MaxStepSize).

@btracey
Copy link
Member

btracey commented Jan 8, 2015

LGTM with the comment changes.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.69%) when pulling 3135309 on first-order-step-size into 022ec44 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.69%) when pulling 3135309 on first-order-step-size into 022ec44 on master.

@vladimir-ch vladimir-ch merged commit 3135309 into master Jan 9, 2015
@vladimir-ch vladimir-ch deleted the first-order-step-size branch January 9, 2015 01:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants