GCVSpline possible missuse

[mitkof6]

I supplied GCVSpline with values of x and y with different lengths accidentally (xN = 151, yN = 150, missed the last value in a for loop). While the calcValue returned normal values, the calcDerivative returned almost infinite numbers and it was difficult to figure out the reason, till I saw that I had supplied less values for y. It would be good if GCVSpline uses arrays instead of *double and double checks the dimensions of the supplied data.

[chrisdembia]

Thank you for reporting. That sounds like a good approach for a fix. Also, this class should be updated to use the new property system (OpenSim_DECLARE_PROPERTY_*) instead of the deprecated property system.

[mitkof6]

@chrisdembia I think I misused GCVSpline all this time. I created the GCVSpline and then called calcValue and calcDerivative. I expected to get smooth values, instead I was getting a noisy curve and I was getting some errors in my algorithm because of it. Fortunately, I begin to plot these curves and saw where the problem was after a lot of debugging. Also, I spotted the corresponding simbody gcv:

SimTK::SplineFitter<Vec3> fitter = SimTK::SplineFitter<Vec3>::fitFromGCV(5, time, position);
SimTK::Spline_<Vec3> spline = fitter.getSpline();

So this seems to work fine with what I wanted to do. Also, I noticed that GCVSpline has a method createSimTKFunction () which does call fitFromGCV, but returns a simbody function and not OpenSim. I think that this method should be made to return OpenSim::Function so that it can be appended in the FunctionSet for example.

As it is now GCVSpline does not creates smooth curves, and I think this is not what a user is expecting. fitFromGCV should be called in the constructor and not to be called manually. Also OpenSim and SimTK function should be made to correspond.

[chrisdembia]

I think that this method should be made to return OpenSim::Function so that it can be appended in the FunctionSet for example.

The way the Function class hierarchy is structured is that all OpenSim functions are implemented via SimTK functions. One could argue that createSimTKFunction() could/should be private. If you want to create a FunctionSet, you can use the constructor of the OpenSim Function classes to populate it.

As it is now GCVSpline does not creates smooth curves, and I think this is not what a user is expecting. fitFromGCV should be called in the constructor and not to be called manually.

The constructor to GCVSpline states:

• @param aErrorVariance Estimate of the variance of the error in the data
  • to be fit. If negative, the variance will be estimated. If 0.0, the
  • fit will try to fit the data points exactly- no smoothing.

It seems you think the default value should be negative (e.g., -1). I think that makes sense; after all, that is Simbody's behavior. However, it would be backwards-incompatible to change the default behavior. So I'm not sure what the best action is.

I suggest you still use the OpenSim::Function, but just specify the error variance as -1 (or something other than 0).

My opinion is: deprecate the current constructor and create a new constructor that takes std::vectors or SimTK::Vectors for the data, and for this new constructor, have the default error variance be negative (so that it is estimated from the data). Though, of course, I think we should wait till we hear what others think.

[mitkof6]

    @param aErrorVariance Estimate of the variance of the error in the data
        to be fit. If negative, the variance will be estimated. If 0.0, the
        fit will try to fit the data points exactly- no smoothing.

It is very easy to skip this.

[mitkof6]

It seems you think the default value should be negative (e.g., -1). I think that makes sense; after all, that is Simbody's behavior. However, it would be backwards-incompatible to change the default behavior. So I'm not sure what the best action is.

Maybe GCVSpline should not have a default value.

virtual SimTK::Function* createSimTKFunction() const = 0;

Seems it is a virtual method.

If you want to create a FunctionSet, you can use the constructor of the OpenSim Function classes to populate it.

Yes but SimTK::Function_ can be temporized, while OpenSim has a data member SimTK::Function, which is constraining.

[clnsmith]

+1 I made the same mistake with the aErrorVariance parameter. It would be nice to be able to access this from a function after construction.

I am tracking a bug where I am getting nan in my GCVSpline coefficients and I think this is related.