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

Bugfix draw_from_gwb_log_uniform_distribution #221

Conversation

Wang-weiYu
Copy link
Contributor

To fix the potential bug described in issue #220

  1. add a parameter dictionary in the JumpProposal class.

  2. use the parameter name as a key to retrieve the parameter, instead of relying on an index within function draw_from_gwb_log_uniform_distribution() in enterprise_extensions.sampler.JumpProposal.

This modifications seem to avoid potential issues associated with index usage.

@vhaasteren
Copy link
Member

This PR looks fine to me. It doesn't pass the linting check though, because of some whitespace:

./enterprise_extensions/sampler.py:542:1: W293 blank line contains whitespace
make: *** [Makefile:39: lint] Error 1
Error: Process completed with exit code 2.

If you delete the whitespace on that line all the checks will likely pass.

@codecov-commenter
Copy link

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (541dd1f) 37.23% compared to head (2b93550) 37.29%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
+ Coverage   37.23%   37.29%   +0.05%     
==========================================
  Files          20       20              
  Lines        3969     3974       +5     
==========================================
+ Hits         1478     1482       +4     
- Misses       2491     2492       +1     
Files Coverage Δ
enterprise_extensions/sampler.py 38.29% <57.14%> (+0.28%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 541dd1f...2b93550. Read the comment docs.

@Wang-weiYu
Copy link
Contributor Author

@vhaasteren Thanks for pointing out this to me and testing my modifications! Now, it is fine.

@vhaasteren vhaasteren requested review from AaronDJohnson and vhaasteren and removed request for AaronDJohnson December 21, 2023 18:28
Copy link
Member

@vhaasteren vhaasteren left a comment

Choose a reason for hiding this comment

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

Looks fine to me now, thanks for fixing this

@vhaasteren vhaasteren merged commit 59b81a1 into nanograv:master Dec 21, 2023
10 checks passed
@Wang-weiYu Wang-weiYu deleted the bugfix_draw_from_gwb_log_uniform_distribution branch December 22, 2023 21:55
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.

3 participants