-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Getting rid of the LinearStateSpace argument in Kalman #354
Comments
Yeah, fair point. I had a look at the code and the Kalman class never actually uses any of the functionality of the Plus, as you say, if we do need such functionality down the road we can always build an instance of LSS inside the class. Could you please put together a PR that closes this issue? We'll also need to coordinate on changing the lecture. Although that shouldn't happen until the next release of the library. (CC @mmcky ) |
Thanks @jstac and @natashawatkins. |
I also think this is a reasonable thing to do. If you wanted to make sure you going from |
Lets talk with John before changing this please. It makes sense not to
change it. Tom
On Oct 23, 2017 9:40 AM, "Chase Coleman" <[email protected]> wrote:
I think this is a reasonable thing to do.
If you wanted to make sure you going from LSS to Kalman was low cost then
one natural thing to do would be to add a method to LSS that created a
Kalman object.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#354 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD22Tja44_AD_jKMYgQKnfS8vwsk3Fotks5svKVSgaJpZM4QA4-8>
.
|
Hi @thomassargent30, I think @natashawatkins has a good point here: To build an instance of If in the future we need to use the functionality of So on balance I support the change proposed by @natashawatkins. |
Go with what John and Natasha think is best. There will be some general
equilibrium effects down the line because we have built some
notebooks (in the pipeline) about the Kalman filter that use the current
protocol.
…On Mon, Oct 23, 2017 at 11:07 AM, John Stachurski ***@***.***> wrote:
Hi @thomassargent30 <https://github.com/thomassargent30>, I think
@natashawatkins <https://github.com/natashawatkins> has a good point
here: To build an instance of Kalman you need to know about
LinearStateSpace. This makes sense on a mathematical level but forms a
barrier to entry when coding, since users need to look up the documentation
for two classes.
If in the future we need to use the functionality of LinearStateSpace
inside Kalman, we can build an instance of LinearStateSpace inside Kalman,
rather than outside.
So on balance I support the change proposed by @natashawatkins
<https://github.com/natashawatkins>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#354 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD22Tv6sPH7FYqWxRaD0t_Cs7x6HWwKYks5svNXvgaJpZM4QA4-8>
.
|
We might be able to use a |
@mmcky Thanks, great suggestion. @thomassargent30 This is a neat suggestion that will allow us to initialize the class with either the A, B, C matrices (for ease of use) or a corresponding |
Wouldn't it be easier to just have the arguments that a
LinearStateSpace
object takes available when creating aKalman
object? It's somewhat cumbersome to first create the LSS object and then put that intoKalman
(you also need to import two classes).We could just create a LSS object using the arguments in
Kalman
.@jstac
The text was updated successfully, but these errors were encountered: