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

GaussianNoiseModel: avoid calling DblNormal with invalid standard deviation #396

Merged
merged 2 commits into from
Oct 27, 2023
Merged

GaussianNoiseModel: avoid calling DblNormal with invalid standard deviation #396

merged 2 commits into from
Oct 27, 2023

Conversation

oysstu
Copy link
Contributor

@oysstu oysstu commented Oct 13, 2023

🦟 Bug fix

Fixes #395

Summary

The fix checks for zero standard deviation before sampling the normal distribution. If the standard deviation is zero, the mean is set directly. I've fixed both cases as mentioned in #395, as I think a crash is undesirable for both cases, but perhaps a warning should be emitted in Load() if the state standard deviation is zero.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@oysstu oysstu requested a review from iche033 as a code owner October 13, 2023 10:05
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #396 (e95f6f2) into main (b834d76) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head e95f6f2 differs from pull request most recent head 1216651. Consider uploading reports for the commit 1216651 to get more accurate results

@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
+ Coverage   72.34%   72.36%   +0.01%     
==========================================
  Files          37       37              
  Lines        5052     5055       +3     
==========================================
+ Hits         3655     3658       +3     
  Misses       1397     1397              
Files Coverage Δ
src/GaussianNoiseModel.cc 98.38% <100.00%> (+0.08%) ⬆️

@iche033
Copy link
Contributor

iche033 commented Oct 16, 2023

changes look good to me.

@scpeters, Perhaps we should add a check in gz::math::Rand::DblNormal to make sure standard deviations are greater than 0? As pointed out in https://stackoverflow.com/a/9186515:

explicit normal_distribution(RealType mean = 0.0, RealType stddev = 1.0);
Requires: 0 < stddev.

@iche033 iche033 merged commit ac0c44f into gazebosim:main Oct 27, 2023
3 checks passed
@oysstu oysstu mentioned this pull request Mar 15, 2024
7 tasks
azeey pushed a commit that referenced this pull request Mar 15, 2024
Signed-off-by: Øystein Sture <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants