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

Add option use_poisson (or not) in generate_random_noise #92

Merged
merged 2 commits into from
Jul 24, 2018
Merged

Conversation

julienguy
Copy link
Contributor

numpy.random.poisson has the drawback of using a varying number of random numbers depending on the mean value (the argument of the function). this is problematic for some applications like testing the effect of DLA on the Lya correlation function where one would like to preserve the random numbers used to simulate the instrumental noise when adding DLAs or not. This PR fixes this by allowing the use of numpy.random.normal instead of numpy.random.poisson in specsim.simulator.Simulator.generate_random_noise.

…d use numpy.random.normal instead of numpy.random.poisson if False
@julienguy julienguy requested a review from dkirkby July 20, 2018 23:28
@@ -562,7 +562,7 @@ def simulate(self, sky_positions=None, focal_positions=None,
# Zero our random noise realization column.
output['random_noise_electrons'][:] = 0.

def generate_random_noise(self, random_state=None):
def generate_random_noise(self, random_state=None, use_poisson=True):
"""Generate a random noise realization for the most recent simulation.

Fills the "random_noise_electrons" column in each camera's output
Copy link
Member

Choose a reason for hiding this comment

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

Missing a docstring description of the new parameter.

random_state.poisson(mean_electrons) - mean_electrons +
random_state.normal(scale=output['read_noise_electrons']))
else :
output['random_noise_electrons'] = random_state.normal(scale=np.sqrt( mean_electrons*(mean_electrons>0) + output['read_noise_electrons']**2))
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add the (mean_electrons>0) since this is not necessary for the Poisson case? mean_electrons >= 0 should always be true here (or else there is a bug). Are you concerned about mean_electrons == 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was just to make sure we don't add a negative variance. but it is always positive or null I remove this test.

@dkirkby
Copy link
Member

dkirkby commented Jul 21, 2018

I checked that scipy.stats.poisson.rvs() has the same behavior, so there is no easy alternative there. In general, reproducible random numbers with different inputs are not part of the low-level np.random contract, so this solution relies on (safe) assumptions about how np.normal is implemented.

@dkirkby
Copy link
Member

dkirkby commented Jul 21, 2018

The failing travis test has been diagnosed and fixed on #91 so does not need to block this PR.

@dkirkby
Copy link
Member

dkirkby commented Jul 24, 2018

Looks good, thanks.

@dkirkby dkirkby merged commit cf637d9 into master Jul 24, 2018
@dkirkby dkirkby deleted the no-poisson branch July 24, 2018 07:36
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.

2 participants