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

Basic simplex optimiser #1

Merged
merged 29 commits into from May 18, 2022
Merged

Basic simplex optimiser #1

merged 29 commits into from May 18, 2022

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented May 9, 2022

No description provided.

@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@a5da4a7). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #1   +/-   ##
=======================================
  Coverage        ?   96.06%           
=======================================
  Files           ?        3           
  Lines           ?      127           
  Branches        ?       25           
=======================================
  Hits            ?      122           
  Misses          ?        5           
  Partials        ?        0           

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 a5da4a7...6a9f56e. Read the comment docs.

@richfitz richfitz marked this pull request as ready for review May 16, 2022 14:27
Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

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

Looks great to me, not having any huge insight into the algorithm. Just a couple of questions about the implications for wodin.

There seem to be a couple of packages on npm doing roughly this already - but they don't offer the ability to hook into each iteration?

README.md Outdated
Comment on lines 28 to 32
while (true) {
if (opt.step()) {
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while (true) {
if (opt.step()) {
break;
}
}
while (!opt.step()) {
// do something
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get data from opt in the while loop to peek at the state during the optimisation? Oh, looking at the example below, looks like you need to make an additional simplex call on opt to get the state out? So the global function simplex runs the whole optimise, but the class method simplex just gives you the current state? That's a little confusing, maybe that could be called something different since the class is already called Simplex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, I'll rename the free function fitSimplex or something like that perhaps

README.md Outdated
* The `step()` method advances the algorithm one step, which will take one or two evaluations of the target function and may or may not find a better point than our current best. It returns `true` if we have converged.
* The `result()` method returns the object described above.

Sometimes, it is useful to get additional information out of the target function and store it alongside the solution. For example, if fitting a model to data you might end up with a sum-of-squares error used for target fitting, but the model values themselves are of interest. For linear regression (which you would fit with a different packge!) we might do this by returning a closure that could be evaluated at any `x` position:
Copy link
Contributor

Choose a reason for hiding this comment

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

So the target function can either just return a numerical value, or an object with interface {value: number, data: any} where data could be any arbitrary data, or a closure or whatever you like? For odin, will we use this? The current parameter values being used will be given in location? And then we could run the model as we usually do with those values to plot the current solution, or would that be returned in data to save having to run it again?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the target function can either just return a numerical value, or an object with interface {value: number, data: any} where data could be any arbitrary data, or a closure or whatever you like?

Yes, exactly that. I did look at requiring the more complex type but it's pretty ugly. Working on an example at the moment that uses the full return type and it seems to work ok.

So the target function can either just return a numerical value, or an object with interface {value: number, data: any} where data could be any arbitrary data, or a closure or whatever you like? For odin, will we use this? The current parameter values being used will be given in location? And then we could run the model as we usually do with those values to plot the current solution, or would that be returned in data to save having to run it again?

Basically, yes - there will be no need to run the model with those parameters again

Choose a reason for hiding this comment

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

Weird, it swallowed my comment. Which was, I didn't quite follow the comment in parentheses - you're saying that you've used linear regression as a toy example but wouldn't actually recommend using this package for linear regression? And I was initially confused about the expected scope of x and y in theta below, maybe include the x and y arrays in the example to make that clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, one should definitely not use this for linear regression! I've updated the comment and added arrays to make that less confusing

Choose a reason for hiding this comment

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

perfect, thanks

}

function showSimplex(id, obj) {
var points = obj.simplex().map(el => el.location);
Copy link
Contributor

Choose a reason for hiding this comment

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

simplex returns an array, not just the current state? Oh, because it includes all the points in the simplex, not just the best one? So what should we be showing on the fit plot at that point? the solution for the first location in the simplex is always the best so show that?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, first is always the best. But it's not that exciting to look at and I was curious how the simplex itself evolves. You can see it conforming to the shape of the surface, even though it does not explicitly have gradient information, which is cool

package.json Outdated
],
"scripts": {
"build": "tsc",
"test": "nyc mocha",
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use jest for js testing, but don't mind giving mocha a try!

Copy link
Member Author

Choose a reason for hiding this comment

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

I am 100% happy to move to what we use currently - could you point me at something similar to crib off? I can move it over to ts tests too at the same time if that's what everything else uses

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe check out vue-charts, that's doesn't have too much unnecessary guff in jest.config.js: https://github.com/reside-ic/vue-charts (You won't need jest-canvas-mock)

Copy link
Member Author

Choose a reason for hiding this comment

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

moved these over, not massively different in the end, which was nice

src/simplex.ts Outdated
}

export function simplex(target: TargetFn, location: number[],
control: Partial<SimplexControlParam> = {},
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to mention control and maxIterations params in the README.

@@ -0,0 +1,30 @@
"use strict";
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests could be ts too if you like

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 done now

richfitz and others added 2 commits May 18, 2022 09:56
Co-authored-by: EmmaLRussell <44669576+EmmaLRussell@users.noreply.github.com>
README.md Outdated Show resolved Hide resolved
Co-authored-by: EmmaLRussell <44669576+EmmaLRussell@users.noreply.github.com>
@richfitz richfitz merged commit 6172b3e into main May 18, 2022
@richfitz richfitz mentioned this pull request Jun 27, 2022
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.

None yet

3 participants