-
Notifications
You must be signed in to change notification settings - Fork 0
Pdaf ipda refactor #33
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 84.75% 84.88% +0.13%
==========================================
Files 37 37
Lines 1882 1813 -69
Branches 1214 1220 +6
==========================================
- Hits 1595 1539 -56
+ Misses 57 50 -7
+ Partials 230 224 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| static std::tuple<Gauss_x, Gauss_z, double> predict( | ||
| const DynModT dyn_mod, | ||
| const SensModT& sens_mod, | ||
| double dt, | ||
| const State& state_est_prev, | ||
| const Arr_zXd& z_measurements, | ||
| Config& config) { |
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.
Pass dyn_mod as reference here perhaps?
Also, using strong typing would make it much easier to use such a function IMO. Something like this maybe (also put the models together to decrease the number of inputs)
static PredictedState predict(const Models& models, const double dt, const State& prev_est_state, const Arr_zXd& z_measurements, Config& config)Another thing, does it make sense to move the config update to a function call after this function is called, since it takes inputs and outputs to/from predict anyway. Reason; I would not expect a function named predict to also update one of the inputs
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.
The same applies to update below
| double P_s = prob_of_survival; | ||
| return P_s * r_km1; // (7.28) |
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.
guess it would be nice to state the source as well, not just the equation number 😅
| static double estimate_clutter_intensity(const Gauss_z& z_pred, | ||
| double existence_prob_pred, | ||
| double num_measurements, | ||
| Config config) { |
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.
Make config const ref perhaps
Andeshog
left a comment
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.
The comments apply up to several places btw, didnt want to repeat too much. And other than that LGTM (Approval with suggestions)
Split the steps function of PDAF and IPDA into separate predict and update functions.
Added z_likelihood function.
Removed redundant z_likelihood calculation in IPDA.
Fixed this if-statement bug that negated the function of the config arg.