Skip to content
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

Adding measure_with_innov_cov. #43

Merged
merged 7 commits into from
Nov 27, 2024
Merged

Adding measure_with_innov_cov. #43

merged 7 commits into from
Nov 27, 2024

Conversation

jihodori
Copy link
Collaborator

Need to add test code

@jihodori jihodori changed the title Adding measure_with_innov_cov. [WIP] Adding measure_with_innov_cov. Oct 24, 2024
Copy link
Member

@zsunberg zsunberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the inline comments, this looks good to me!

@@ -19,6 +19,18 @@ function update(filter::AbstractFilter, b0::GaussianBelief,
return bn
end

function update_with_info(filter::AbstractFilter, b0::GaussianBelief,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just be update_info

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't update_info suggest that the function is simply updating some information? However, the function is actually updating the filter and returning the updated GaussianBelief with information

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is a valid objection, and I am glad that it bothers you. I suggested update_info because it matches POMDPs.update_info. I do not remember exactly why we chose that over update_with_info, probably simply for conciseness. In any case, this is a subjective choice, so you can decide what to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely measure_info and update_with_info should have the same form. A docstring can easily explain what the function does.

src/kf.jl Outdated
# measure
bn, Σ_Y = measure_info(filter, bp, y; u = u)

return bn, Σ_Y
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be called info I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed

@zsunberg zsunberg changed the title [WIP] Adding measure_with_innov_cov. Adding measure_with_innov_cov. Nov 14, 2024
Copy link
Member

@zsunberg zsunberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you decide about update_info vs update_with_info, I think this is OK to merge. Really, those functions should have docstrings, but I don't want to hold you back too much.

@@ -19,6 +19,18 @@ function update(filter::AbstractFilter, b0::GaussianBelief,
return bn
end

function update_with_info(filter::AbstractFilter, b0::GaussianBelief,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely measure_info and update_with_info should have the same form. A docstring can easily explain what the function does.

@jihodori jihodori merged commit 64983be into master Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants