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

Issue 404 - refactoring simulation.py #408

Merged
merged 6 commits into from
Oct 29, 2019
Merged

Conversation

sbenthall
Copy link
Contributor

This PR is for refactoring of simulation.py as per #404

Currently it:

  • adds a unit test for each method in simulation.py
  • refactors drawMeanOneLognormal to use drawLognormal

I would like to go farther and refactor more of the distribution sampling code to be more concise.
But I'm making this PR mainly to test project workflow.

@@ -33,19 +33,13 @@ def drawMeanOneLognormal(N, sigma=1.0, seed=0):
# Set up the RNG
RNG = np.random.RandomState(seed)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't needed in this function as the seed is passed on to drawLognormal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call


class SimulationTests(unittest.TestCase):
'''
Tests for ....
Copy link
Member

Choose a reason for hiding this comment

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

sampling distribution with default seed?

PS: looks like import sys isn't used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

@sbenthall
Copy link
Contributor Author

I corrected based on @MridulS 's feedback.

Based on today's meeting, my understanding now is that simulation.py code is deprecated and slated to be replaced with dolark in the near future.

So I won't do any more refactorings in this PR. I submit this PR to be reviewed and pulled.

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.

4 participants