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

Conversation

btracey
Copy link
Member

@btracey btracey commented Aug 11, 2016

No description provided.

@btracey
Copy link
Member Author

btracey commented Aug 11, 2016

@vladimir-ch Here is another vision of the global optimization routine. Like last time, it's in a 30% done stage. Rather than detailed comments, looking at opinions on high-level structure (sufficiently clean, think it'll expand, etc.)

global.go Outdated
func (g *globalStatus) globalOperation(op Operation, loc *Location, x []float64) Status {
// TODO(btracey): Combine this with Local
stats := g.stats
mux := g.mux
Copy link
Member

Choose a reason for hiding this comment

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

(I know you only want high-level comments, but bear with me...)

copying sync.Mutex is bound to raise eyebrows.
I know here we just copy a *sync.Mutex but... couldn't we remove this line and just access the mutex via g.mux ?
I'd feel much less woried about a possible modification of globalStatus down the line...

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. The original code had mux as a function input, and so this was the easiest change to get something to compile. This way seemed slightly easier to get the flavor of what's going on, so I left it. This will definitely be changed.

@vladimir-ch
Copy link
Member

On high level, I like what is in here, I can't think of anything I would change in the important parts of the API, Global and GlobalMethod.

On low implementation level, I think we should and can avoid the synchronization via mutex, and we need to figure out the error handling.

@btracey
Copy link
Member Author

btracey commented Aug 12, 2016

We should reduce the mutex overlap as much as possible, but the fundamental problem is unavoidable. The main thing is to make sure function evaluations aren't blocking other function evaluations, which is the case here.

@btracey
Copy link
Member Author

btracey commented Aug 23, 2016

PTAL @vladimir-ch

This is a full implementation, I think. There is obviously lots of parallel between this and Local, which is good. However, the two are subtly different in ways that make it difficult to simply combine the two. What I would like to do in follow-on PRs is:
0) Get this change (or something like it submitted)

  1. Significantly combines the implementations of Local and Global
  2. Add better tests for Global, and improve comments where needed.
  3. Add a "real" global optimizer

The above a) Isolates adding Global from messing up local. b) Reduces unnecessary comment work. If you disagree with the spirit of b, I can make a bunch of comment improvements in this PR.

One open question is if OptLoc should always be updated on major iteration, like it does in Local. I lean toward yes for consistency (changing both here and Global).

I think I disagree with avoiding a mutex. We have to have serialization to update stats, etc. This has to be done with a mutex or channel. It's not clear that a channel is at all better. This version at least has no blocking during function evaluation which is the main thing.

errors.go Outdated
// due to floating-point arithmetic.
ErrNoProgress = errors.New("linesearch: no change in location after Linesearcher step")

ErrFunctionConvergeNil = errors.New("global: function convergence must not be nil")
Copy link
Member

Choose a reason for hiding this comment

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

This error is not used anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it should be just a panic string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleted.

@vladimir-ch
Copy link
Member

I've given it a quick pass and LGTM with minor comments, I think it turned out quite nicely.

I've ignored the comments, please improve them later.

One open question is if OptLoc should always be updated on major iteration, like it does in Local. I lean toward yes for consistency (changing both here and Global).

Yes, I think it should be on major iteration like it is in Local. There we agreed that it is the optimizer's responsibility to tell us when the location is complete and ready for a convergence check. Here I think the situation is the same.

I think I disagree with avoiding a mutex. We have to have serialization to update stats, etc. ...

It is ok to have the mutex.

@btracey btracey merged commit 10cf311 into master Sep 1, 2016
@btracey btracey deleted the globaltrytwo branch September 1, 2016 05:07
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.

3 participants