Chebyshev2 Improvements #1660
Chebyshev2 Improvements #1660
Conversation
|
I wish you had done a design review with me first. I don’t support the change in the meaning of N without extensive discussion… |
I've been using Chebfun and the mental gymnastics of relating that to our implementation was getting to me. I believe we were wrong to have polynomials of degree N (instead of N+1) for the specified degree N. |
|
@dellaert as discussed offline, I have reverted the change to go back to using order N polynomials (degree N-1) instead of order N+1 (degree N). |
gtsam/basis/Chebyshev2.h
Outdated
| @@ -83,6 +86,11 @@ class GTSAM_EXPORT Chebyshev2 : public Basis<Chebyshev2> { | |||
| return points; | |||
| } | |||
|
|
|||
| /// Return a zero initialized Parameter matrix. | |||
| static Parameters ParameterMatrix(size_t N) { | |||
There was a problem hiding this comment.
I don't see why we can't just say Zero(N) ?
There was a problem hiding this comment.
Yes this needs to be removed since it was added for the previous change of N to N+1. Removing.
gtsam/basis/Chebyshev2.h
Outdated
| * | ||
| * Overriden for Chebyshev2. | ||
| */ | ||
| static Matrix WeightMatrix(size_t N, const Vector& X, double a = -1, |
There was a problem hiding this comment.
Where is this used? Just being worried about this MxN matrix being confused with the other MxN matrix that is used for VectorEvaluators. But I remember we talked about this as a speedup in some scenarios?
There was a problem hiding this comment.
This needs to be removed too.
|
@dellaert okay addressed all comments. :) |
According to Trefethen, given N, we should form a polynomial with N+1 nodes, but we form a polynomial with N nodes (which is mathematically a N-1 degree polynomial).
Hence one of the bigger changes of this PR is to form a polynomial over N+1 nodes. This makes our code correspond directly to the math and makes comparison to Chebfun much easier.
Additionally, I've tightened some bounds checks and improved the overall API.