-
Notifications
You must be signed in to change notification settings - Fork 6
State Space Representations #181
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
Conversation
d077032
to
c94082a
Compare
5d8a6ea
to
52c0d64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I see the application from this example. The information is available from the parameter value and the features argument.
double x = min; | ||
while (x < max) { | ||
output.push_back(x); | ||
x += step; | ||
} | ||
output.push_back(max); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would loop this over n
instead of a while. I think it makes it easier to read and allows the addition of a number close to max
to be added in the loop.
edit: or use the linspace
function moved in this commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, yeah forgot to straighten that all out. I used the linspace method first (that's why I moved it) but that required casting n
to a std::size_t
which probably isn't much of an issue (we do it elsewhere) but I do think there is an advantage to avoiding that as it could cause overflow which is how I landed where it currently is.
auto _ssr_impl(const std::vector<X> &xs) const { | ||
return concatenate(this->lhs_.state_space_representation(xs), | ||
this->rhs_.state_space_representation(xs)); | ||
this->rhs_.state_space_representation(xs)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't help but feel like concatenation is the wrong operation.
Imagine adding two squared exponentials of different length_scale. They'll have the same range, but the number of inducing points coming from each of them will depend on the length_scale. You could get duplicate inducing points (midpoint?) or ones in close proximity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not bullet proof yet. I would prefer to not pass around std::set
since everywhere else uses std::vector
... but maybe it's more appropriate. Alternatively we could filter out all duplicates in the state_space_representation
method, but that would result a lot of repeated work. For now I think I'm OK leaving it up to the user to realize there are duplicates and decide if that's an issue.
// using 1/10th of the length scale should result in grids with | ||
// one percent decorrelation between them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't users want different granularity depending on application?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good chance, I do think this is a relatively disciplined approach though and if users want something different they can use their own InducingPointStrategy
@seth-swiftnav Imagine a situation where you composed a lot of covariance functions together, something like:
If you wanted to use the That explain the use application more? |
@akleeman I'm wondering if the ssr_impl_ of the smaller covariance functions is going to produce a better set of induction points than the designer of the top level covariance functions. I forsee a smaller covariance function not having an ssr_impl_ and the concatenated induction points not spanning the top level function. |
@seth-swiftnav I think those are fair concerns, I certainly don't expect the default |
state_space_representation() method from a covariance function directly. Regardless of whether that is used it also gives the inducing point strategy access to the covariance function which opens up a lot of possibilities such as inspecting parameters.
52c0d64
to
05e3d46
Compare
91d7b60
to
b3bc75c
Compare
b3bc75c
to
d0ede7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! I have some trouble following the templating/trait syntax, but overall I have no new comments/concerns.
/* | ||
* SquaredExponential distance | ||
* - c(d) = -exp((d/length_scale)^2) | ||
* covariance(d) = sigma**2 exp(-(d/length_scale)^2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: mixing **
and ^
. I think carrot is more recognizable, as **
is python/java? (even if ^ is xor in C)
…on (#181) (#511) Automated PR by Jenkins. If CI has passed successfully, merge away! **cmake** 30226aa8 -> 8920f2a3 - 8920f2a3 : Fix importing of latest protobuf version (swift-nav/cmake#181) This pull request was created by https://jenkins.ci.swift-nav.com/job/CI%20Infra/job/submodule-update/19843/.
This change adds another optional functionality to covariance functions, namely it let's you have the covariance function directly tell you what set of states should be able to capture it's behavior.
For example, the
SquaredExponential
covariance function now has an_ssr_impl
method which uses it's length scale to determine the spacing of a grid that you'd need to reasonably approximate what the full covariance would capture. This is used and tested intest_sparse_gp.cc
.Another example: