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

[DEP][DOC}: 696 fix random seed documentation #25

Conversation

bretsnev
Copy link
Contributor

Changes:

  1. Replaced all instances of np.random.seed(int) with seed(int)
  2. Where relevant added: from bilby.core.utils.random import seed, rng, with no rng if its not needed.
  3. Added seed(int) to "examples/gw_examples/injection_examples/create_your_own_source_model.py", as it uses rng, but no seed was set as in other examples.
  4. Replaced documentation that asked for legacy numpy.random.state, instead asking for numpy random generator instance and a note about built in bilby.core.utils.random.rng

notes:

  1. Other instances of np.random... are still in use in the tests instead of using new generator instance.

@GregoryAshton
Copy link
Collaborator

Thanks @bretsnev this looks great. I reviewed over and I don't see any issues.

@ColmTalbot should we set the target branch to github-main?

@ColmTalbot
Copy link
Collaborator

@ColmTalbot should we set the target branch to github-main?

Yeah, there's a drop-down in the edit options in the description.

@ColmTalbot
Copy link
Collaborator

This should wait until after we merge #24 as I suspect there are style issues that would fail the pre-commits.

Copy link
Collaborator

@ColmTalbot ColmTalbot left a 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.

bilby/core/sampler/dynesty_utils.py Outdated Show resolved Hide resolved
examples/tutorials/compare_samplers.ipynb Outdated Show resolved Hide resolved
@bretsnev
Copy link
Contributor Author

bretsnev commented Feb 24, 2024

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 black formating? Is this just something to be aware of if he added/removed lines get flagged?

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.

@ColmTalbot ColmTalbot force-pushed the 696-fix-random-seed-documentation branch from a6c001e to d3e3cc5 Compare February 24, 2024 14:15
@ColmTalbot ColmTalbot changed the base branch from master to github-main February 24, 2024 14:16
@ColmTalbot
Copy link
Collaborator

Thanks @bretsnev, there are some instructions for setting up the pre-commits here. After running pre-commit install you can run pre-commit run --all-files and it should fix any issues you have with the formatting.

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.

@ColmTalbot ColmTalbot force-pushed the 696-fix-random-seed-documentation branch from d3e3cc5 to 2ce19fe Compare February 24, 2024 19:27
@ColmTalbot
Copy link
Collaborator

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).

@GregoryAshton
Copy link
Collaborator

@ColmTalbot: I can sit with @bretsnev and get the pre-commits set up to fix these issues.

@bretsnev bretsnev requested a review from ColmTalbot March 6, 2024 13:59
@bretsnev
Copy link
Contributor Author

bretsnev commented Mar 6, 2024

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.
@GregoryAshton
Copy link
Collaborator

Closing this in deference to #28

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.

5 participants