Upper and Lower bounds for optim() method might be redundant

[KathrinTessella]

While rewriting the movement() method, I found a redundant code section / potential bug when calling the optimization method optim().

In line 120/121 of movement.R in the movement() function body there are two different calls to the attemptoptimisation() method whereas one is comment out:
L120: #optimresults <- attemptoptimisation(predictionModel, locationdataframe, movement_matrix, progress=FALSE, hessian=TRUE, upper=upper, lower=lower, ...) #, upper=upper, lower=lower
L121: optimresults <- attemptoptimisation(predictionModel, population_data, movement_matrix, progress=FALSE, hessian=TRUE, ...) #, upper=upper, lower=lower

Using the call in line 121 (which does NOT assigns the upper and lower arguments) results in the scenario that the optim() method will use the default values for lower (=-Inf) and upper (=Inf) [see documentation of optim()] and NOT the upper / lower bounds set in the line 59..103 in the movement() method depending on the model selected. This option was in the movement.R package as I started the project.

Using the call in line 120 (which assigned the upper and lower argument to the attemptoptimisation() method) will result that the upper and lower values, as set in line 59..103 in the movement() method depending on the model selected, will be passed over to the optim() method. This will, however, cause an error, as the bounds can ONLY be used for the optimization methods = "L-BFGS-B" or "Bent" - but we use the 'BFGS" method.

Thus, if the "BFGS" method should be use for the optimization, the 'upper' and 'lower' parameters are redundant and the code should be cleaned accordingly (including the relevant tests). Furthermore, the movement() method MUST use the call
optimresults <- attemptoptimisation(predictionModel, population_data, movement_matrix, progress=FALSE, hessian=TRUE, ...)
and the other comment-out line should be removed to avoid future confusion.

If however, the upper and lower bounds should be used by the optimization model, then the code contains a bug and needs fixing.

[goldingn]

This is because some of the parameters for some of the models have fixed support...

A better (i.e. more stable) approach would be to define transformations for the parameters, and optimising on their unconstrained transformations. There there would be no need for limits on the optimiser. E.g. for some parameter a which must be positive, we should instead optimise a_t where a = exp(a_t); for some parameter b which must be between 0 and 1, we should optimise b_t where b = plogis(b_t) (the probit transform), so that a_t and b_t are unconstrained and we don't need to deal with upper and lower.
This approach will require defining some transformation functions and their inverses, and explicitly stating for each model (or flux function?) what the transformations are.

I think it would be much better to tackle this now (it's something we discussed previously but Andrew didn't get to), rather than fudge the optimiser limits now. Shall we discuss this on Monday?

[KathrinTessella]

Yes - lets discuss this on Monday and I believe its worth spending now the time to address this as the code is currently inconsistent and, thus, unstable.

[goldingn]

Absolutely, there's a lot of cleaning up to do in there :)

[KathrinTessella]

First steps are done in pull request #74

[KathrinTessella]

The second, and final step, are done in pull request #75.

[KathrinTessella]

task completed.