-
Notifications
You must be signed in to change notification settings - Fork 24
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
I12b unscented kalman edits #180
I12b unscented kalman edits #180
Conversation
Update the paper reference, the value of kappa (from 1 to 0), the allowed input values to observe, and the SquareRootUKF functions: init, filtered_cholupdate and cholupdate to cope with downdates as well as updates.
- update model parameters to parameter_set - add n_outputs to BaseCost - remove observer from FittingProblem - change solver to _solver - add evaluate and problem inputs to Observer - except all Exception from call to solver - add dataset to UnscentedKalman - update tests to match
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## i12-unscented-kalman #180 +/- ##
========================================================
+ Coverage 94.43% 94.58% +0.15%
========================================================
Files 38 40 +2
Lines 1617 1756 +139
========================================================
+ Hits 1527 1661 +134
- Misses 90 95 +5 ☔ View full report in Codecov by Sentry. |
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.
Thank you @NicolaCourtier, this looks great :) I have a few suggestion below, they might be more structural things, so happy if we discuss / sort them out later, so I'll hit approve
self.reset(inputs) | ||
|
||
output = [] | ||
if hasattr(self, "_dataset"): |
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.
ideally all attributes of a class should be defined in the init. If some attributes are optional then they should either be set to something, or None
. Then you can test for None
. Otherwise type checking becomes a pain. This is probably an issue with Problem
however, so happy to leave it for later.
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.
Agree- this can be sorted after merge! The decision is whether we want to allow the UKF to be run without a dataset (e.g. for testing?) or make it compulsory as it will usually be required.
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 know I did make that optional, but it is a bit weird to call observe
and not observe anything. If you think it should be compulsory I'm happy to go with that.
Edits to the Observer class and Kalman filter implementation.