-
Notifications
You must be signed in to change notification settings - Fork 9
Add MoreThuente Linesearcher #148
Conversation
linesearcher_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be in functions? Even if specialized I think we/someone will find it useful to have an easily accessible set of functions. We could provide a one-D wrapper here or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I will move them there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL @btracey
4680b16
to
738663a
Compare
errors.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this is "ErrLinesearchBound". Some linesearchers have a minimum step, and this may as well cover both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to ErrLinesearcherBound
. At the moment I will leave it as an ordinary error but I can imagine that storing the step and the bounds could be potentially useful.
3169780
to
f42a908
Compare
Addressed comments. I also added a check that |
functions/functions.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
characteristics such as?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard for me to describe it in one sentence. Please take a look at what I have there now.
Sorry, I do understand why Max and Min steps are absolute, but I don't understand why StepTolerance should be relative, especially when the other Step numbers are all absolute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this only has two values, should it be a boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact it has three values because I (ab)use the zero value of stage
to check that Init
has been called. If you don't like it or if such stringent checking is deemed too pedantic, I can make it a bool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's fine.
f42a908
to
d21c159
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think absolute is easier to think about. There's no clear definition of "relative" here (though I know you provided one). Additionally, the tolerance, even absolutely, is already scaled no ||dir||. Our "coordinates" have a step of size ||dir|| equal to 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, the tolerance, even absolutely, is already scaled no ||dir||. Our "coordinates" have a step of size ||dir|| equal to 1.
Yes, that's why I offered the alternative to measure the interval width in real, non-scaled-on-dir units. However, for that we would have to change the Linesearcher
interface and that does not feel worthy. I don't have a strong argument for a relative tolerance, so I will change it to an absolute tolerance and see what it does with the tests.
morethuente.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
d21c159
to
d70a5c8
Compare
06aca1d
to
14df624
Compare
LGTM. Thanks. |
The discussion about the relative step tolerance has been left open-ended but I will merge anyway. I will open an issue and copy the discussion there so that it is together in one place. |
14df624
to
12b79c6
Compare
I like the names DecreaseFactor and CurvatureFactor. I would vote for using them also in the other Linesearchers.
As I mentioned before, CG is the method that most benefits from this Linesearcher, so I set it as the default. Maybe we should remake the CG tests, there are already many combinations and the Linesearchers add an extra dimension.
PTAL @btracey