-
Notifications
You must be signed in to change notification settings - Fork 71
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
[DEP][DOC}: 696 fix random seed documentation #25
[DEP][DOC}: 696 fix random seed documentation #25
Conversation
Thanks @bretsnev this looks great. I reviewed over and I don't see any issues. @ColmTalbot should we set the target branch to |
Yeah, there's a drop-down in the edit options in the description. |
This should wait until after we merge #24 as I suspect there are style issues that would fail the pre-commits. |
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.
Hi @bretsnev thanks for going through these! I left in a few general comments for minor fixes.
Another thing that I didn't explicitly mention is that in many cases lines have been added/removed, this is another thing that I suspect will be automatically flagged by running the pre-commit checks as black
formatting is applied to all of the examples.
examples/core_examples/alternative_samplers/linear_regression_bilby_mcmc.py
Outdated
Show resolved
Hide resolved
examples/gw_examples/injection_examples/change_sampled_parameters.py
Outdated
Show resolved
Hide resolved
Hi @ColmTalbot, I'm not sure how to run the pre-commit checks here, bit confused with that on github vs looking at the ligo-git. I'm also not familar with Other than that I've made the other changes you mentioned, thank you for spotting the mistakes :) Please let me know ifyou notice anything else I've missed. |
a6c001e
to
d3e3cc5
Compare
Thanks @bretsnev, there are some instructions for setting up the pre-commits here. After running There are some new changes that I added that need to be independently added to the target, branch, I'll do that as soon as possible, sorry for adding more noise to this PR. Our system still has a few issues. |
8137c12
to
c5556bb
Compare
…of legacy generator
d3e3cc5
to
2ce19fe
Compare
I managed to rebase to the version of the target branch that includes the CI tests, you can now see the failures in the CI in the box at the bottom of the PR (one specific example is here). |
examples/core_examples/alternative_samplers/linear_regression_bilby_mcmc.py
Outdated
Show resolved
Hide resolved
@ColmTalbot: I can sit with @bretsnev and get the pre-commits set up to fix these issues. |
Thank you for all the help, I think the requested changes have all been made. If you find any more mistakes or if I've missed/made a mistake with any of the previously requested changes, please let me know. |
I realised that pre-commit didn't check any files because there were no logged chagnes. So I ran pre-commit on everything using "pre-commit run --all-files". This only brought up relevant changes from myself, with the exceptions of: "bilby/bilby_mcmc/sampler.py:525:38: E226 missing whitespace around arithmetic operator" which is outside the remit of this merge request.
Closing this in deference to #28 |
Changes:
np.random.seed(int)
withseed(int)
from bilby.core.utils.random import seed, rng
, with no rng if its not needed.seed(int)
to "examples/gw_examples/injection_examples/create_your_own_source_model.py", as it usesrng
, but no seed was set as in other examples.bilby.core.utils.random.rng
notes:
np.random...
are still in use in the tests instead of using new generator instance.