Repository navigation
Rubin deep coadd simulation code - #521
Conversation
…nsider band-dependence in the internal caches; fix pixel cell center calculation
|
Is this ready for review? You still have it marked as draft. |
|
I was planning to remove the fft stuff before marking it ready, but everything else is ready to be looked at and I expect there will be some iteration anyway, so I'll mark it ready for you to have a look. |
rmjarvis
left a comment
There was a problem hiding this comment.
Looks good Jim. Just a few comments and questions.
| params, safe = galsim.config.GetAllParams(config, base, req=req) | ||
| deep_coadds = galsim.config.GetInputObj('deep_coadds', config, base, | ||
| 'RubinDeepCoaddBandpass') | ||
| return deep_coadds.getBandpass(params['band']), safe |
There was a problem hiding this comment.
Why is this a special deep_coadds version? This looks like it's just the normal LSST bandpass we normally use. Am I missing something here?
There was a problem hiding this comment.
This retrieves the "standard passband" from the butler, which is a calibration product. See the butler code here. This was mentioned in a slack thread. The one we've been using for the visit level sims can also have per-detector throughputs included, which we wouldn't use for the coadds.
| """ | ||
| if 'noise' not in base['image']: | ||
| base['image']['noise'] = { "type": "Gaussian", | ||
| "sigma": self.noise_level } |
There was a problem hiding this comment.
Why this bit? I think it would be safer to just make the user include this in the config if they want gaussian noise.
There was a problem hiding this comment.
The object rendering needed to have a noise level in order to have reasonable stamp sizes. I actually wasn't sure if this was the right thing to do, but it seemed to make sense to have a noise level that was informed by the original image. I'm open to alternative ways of handling this.
There was a problem hiding this comment.
I think it would be ok to raise an exception if the noise is missing. Could even have a custom noise type that is just CoaddGaussianNoise or something that would do this, which we could recommend in the error message.
But I think automatically adding this if it's missing is too much magic sauce. Better to be explicit IMO.
| base['image']['noise'] = { "type": "Gaussian", | ||
| "sigma": self.noise_level } | ||
| if self.add_noise: | ||
| AddNoise(base, image, current_var, logger) |
There was a problem hiding this comment.
Also, I think if we're doing this here, then we don't need the self.add_noise attribute. That is only used in the regular LSST_ImageBuilderBase class because FullFocalPlane doesn't add the noise for the second pass of a 2-pass image generation.
| image_pos = base['image_pos'] | ||
| celestial_coord = base['wcs'].toWorld(image_pos) | ||
| ra = celestial_coord.ra / galsim.degrees | ||
| dec = celestial_coord.dec / galsim.degrees |
There was a problem hiding this comment.
I'd recommend leaving these as the celestial_coord and make that the argument to getPSF. The whole point is to avoid implicit definitions of angles being in degrees at various places in the code. Only when finally something requires degrees or radians, convert there.
| xsize = ysize = 32 | ||
| else: | ||
| # Determine the stamp size from the object flux. | ||
| noise_var = deep_coadds.getNoiseVar(ra, dec, band) / 3.0 |
There was a problem hiding this comment.
This / 3.0 probably deserves an explanation.
There was a problem hiding this comment.
This is something else I was a bit unsure about. If I just use the noise from the image, some stamp boundaries are apparent in some galaxy renderings, especially ones that are long and thin. This factor mitigated the problem for the test image I've been using, but I don't know that it would work in general. I'm open to suggestions.
There was a problem hiding this comment.
The variable itself (named noise_var) should probably not take the /3, since that feels like lying to ourselves. :) But we could document when we use it that we make stamps large enough that the profile at the edge drops to 1/3 the noise level to avoid apparent edges.
| # Probably a little better to put them a bit later than the | ||
| # start in some cases (e.g. after TimeSampler, | ||
| # PupilAnnulusSampler), but leave that as a todo for now. | ||
| photon_ops = psfs + photon_ops |
There was a problem hiding this comment.
I believe all this psf as photon_op handling is currently done correctly in GalSim itself. I think this goes back to when this was all new and we didn't have all that implemented correctly. It's probably ok, but I think it likely makes our code more fragile than it needs to be. If we can skip that and just let GalSim do it, that would be better. (Same with where we do this in the regular LSST_Image class.)
There was a problem hiding this comment.
Would the fix be just to omit this line?
There was a problem hiding this comment.
Also to draw the original profile, rather than split out the psfs. So skip the line where we make gal, *psfs from the original profile, and just use prof where we currently use gal. I think that should all just work, but definitely should test it to make sure I'm not missing something.
There was a problem hiding this comment.
Hi Mike, I think I've addressed all of your comments. Please have a look when you get a chance.
…rdinate object instead of ra, dec
…ise type explicitly in the config
…rent stamp edges for long, thin galaxies
… as photon_ops explicitly
No description provided.