mljs / levenberg-marquardt

Curve fitting method in JavaScript
MIT License
70 stars 15 forks source link

Bug with default `initialValues` option #52

Closed opatiny closed 1 year ago

opatiny commented 1 year ago

initialValues option is optional, but if it is not passed, the algorithm systematically returns { parameterValues: [ 1 ], parameterError: NaN, iterations: 0 } as shown in the below example:

export function line([a, b]: number[]): LineFunction {
  return (x: number) => a * x + b;
}

  const x = [0, 1, 2];
  const y = [0, 1, 2];

  const result = levenbergMarquardt({ x, y }, line, {});
 // returns { parameterValues: [ 1 ], parameterError: NaN, iterations: 0 }

  const result = levenbergMarquardt({ x: source, y: destination }, line, {
    initialValues: [0,0],
  });
 // returns  {
 //  parameterValues: [ 0.9999945156042755, 0.0000073091439573630054 ],
 //     parameterError: 7.014631126514258e-11,
 //     iterations: 2
    }
jacobq commented 1 year ago

Interesting...it looks like omitting initialValues is supposed to be equivalent to setting all parameters to 1, but this line seems to assume the parameters are separate arguments, not a single array containing the parameters: https://github.com/mljs/levenberg-marquardt/blob/c8317bc28753976e1182cf9b381080a361d56aaa/src/checkOptions.js#L38 If that's indeed incorrect then it's been a latent bug for a very long time (>5 years if I'm interpreting git blame accurately). Furthermore, I don't see any test cases that omit the initial values. I suggest converting your example into a test case and then making it pass. There will need to be some way for LM to determine how many parameters there are (length of array), and it's not obvious to me what that should be. For now the work-around is to supply initialValues whenever there is more than 1 parameter. It seems a shame to me that an array is passed to the function instead of making each parameter an argument since it is easy to go the other way (parameterizedFunction(...arrayOfParams) vs parameterizedFunction(arrayOfParams)), but everything appears to be already set up to do it this way (and maybe it's also more performant like this) so would be a royal pain & major breaking change to alter that at this point.

Example ```js // example.cjs const { levenbergMarquardt } = require('./lib/index.js'); // working off local build function line([a, b]) { return (x) => a * x + b; } // We want to know the number of parameters, but by looking at the function // we can only see the number of arguments it takes, which will always // be 1 because it takes a single array of arbitrary size. console.log(`LM things this is how many parameters there are -->`, line.length); const x = [0, 1, 2]; const y = [0, 1, 2]; let result; // Running with no initialValues is currently equivalent to settings // initialValues to [1], which causes parameter error to become NaN // in this case, and the algorithm does not run. // --> { parameterValues: [ 1 ], parameterError: NaN, iterations: 0 } result = levenbergMarquardt({ x, y }, line, { initialValues: undefined }); console.log(result); result = levenbergMarquardt({ x, y }, line, { initialValues: [1] }); console.log(result); // Calling with the correct number of initial parameters produces good results result = levenbergMarquardt({ x, y }, line, { initialValues: [0,0] }); console.log(result); // { // parameterValues: [ 0.9999945156042755, 0.0000073091439573630054 ], // parameterError: 7.014631126514258e-11, // iterations: 2 // } result = levenbergMarquardt({ x, y }, line, { initialValues: [1,1] }); console.log(result); // { // parameterValues: [ 0.9999926908560426, 0.000010357158362672703 ], // parameterError: 1.3471834622424516e-10, // iterations: 2 // } // Calling with more initialValues than there are parameters still produces // a valid result, except with excess parameters result = levenbergMarquardt({ x, y }, line, { initialValues: (new Array(100)).fill(1) }); console.log({ ...result, parameterValues: result.parameterValues.slice(0, 2), }); // (same output as previous case) ```
jacobq commented 1 year ago

initialValues option is optional

BTW, could you tell me where you found that information? I see in the code that a default value is supplied if it is omitted, but I didn't see any documentation beyond the jsdoc comment, which does not show it as optional. The simplest way forward may be to make the current behavior explicit, i.e. show that default value is [1] (replacing code that tries to get using function.length) and document that it is not optional when there is more than 1 parameter.

jacobq commented 1 year ago

@opatiny Thanks for reporting this. initialValues was not supposed to be optional. PR #53 makes it so that now an error will be thrown if it is missing.

opatiny commented 1 year ago

@opatiny Thanks for reporting this. initialValues was not supposed to be optional. PR #53 makes it so that now an error will be thrown if it is missing.

Thank you very much for the fix! :)