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 obj.makePhot method #1078

Merged
merged 15 commits into from
May 13, 2020
Merged

Add obj.makePhot method #1078

merged 15 commits into from
May 13, 2020

Conversation

rmjarvis
Copy link
Member

@rmjarvis rmjarvis commented May 8, 2020

This PR adds a way to draw photons without actually accumulating them onto an image.

It is functionally equivalent to _, photons = obj.drawPhot(im, ...) but without requiring the image.

@cwwalter

@rmjarvis rmjarvis requested a review from cwwalter May 8, 2020 03:03
Copy link
Member

@jmeyers314 jmeyers314 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just one question: would it be possible for drawPhot to use makePhot? Too tricky with the resume I'm guessing?

@rmjarvis
Copy link
Member Author

rmjarvis commented May 9, 2020

Just one question: would it be possible for drawPhot to use makePhot? Too tricky with the resume I'm guessing?

That was originally my plan, but with all the maxN and resume stuff, if seemed a little too much to untangle cleanly. This seemed simpler, and the unit tests make sure they are equivalent for their overlapping use cases.

Copy link
Member

@cwwalter cwwalter left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! Two minor comments/question in the review.

Also, just for my education/curiosity: Why all the changes to " rng=rng.duplicate()" in the tests? Was it not giving a reproducible sequence before?

@@ -2067,6 +2067,101 @@ def _calculate_nphotons(self, n_photons, poisson_flux, max_extra_noise, rng):
return iN, g


def makePhot(self, n_photons=0, rng=None, max_extra_noise=0., poisson_flux=None,
sensor=None, surface_ops=(), maxN=None, orig_center=PositionI(0,0),
Copy link
Member

Choose a reason for hiding this comment

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

Sensor isn't in the parameter list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Also maxN. Fixed.

Returns:
- a `PhotonArray` with the data about the photons.
"""
from .sensor import Sensor
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary in this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. No not needed here.

Copy link
Member

@cwwalter cwwalter left a comment

Choose a reason for hiding this comment

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

Forgot to click approve..

@rmjarvis
Copy link
Member Author

Also, just for my education/curiosity: Why all the changes to " rng=rng.duplicate()" in the tests? Was it not giving a reproducible sequence before?

These tests check that drawPhot and makePhot produce the same exact photons. So the first time, for drawPhot, I call rng.duplicate() to fork the RNG sequence. Then when I do it again with makePhot, the rng runs through the exact same sequence again, so should produce the same photons.

@rmjarvis rmjarvis merged commit 9f6cce8 into master May 13, 2020
@rmjarvis rmjarvis deleted the makePhot branch May 13, 2020 05:16
@rmjarvis rmjarvis added this to the v2.3 milestone Jun 11, 2020
@rmjarvis rmjarvis added base classes Related to GSObject, Image, or other base GalSim classes feature request Request for a new feature in GalSim desc Of possible interest to LSST DESC members looking for a project labels Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base classes Related to GSObject, Image, or other base GalSim classes desc Of possible interest to LSST DESC members looking for a project feature request Request for a new feature in GalSim
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants