LSSTDESC / imSim

GalSim based Rubin Observatory image simulation package
https://lsstdesc.org/imSim
BSD 3-Clause "New" or "Revised" License
36 stars 15 forks source link

Add airmass dependent Bandpasses #444

Closed jmeyers314 closed 6 months ago

jmeyers314 commented 7 months ago

Work in progress, looking for feedback.

Adds

I'm still not sure how to:

I'll dig in tomorrow, but any advice appreciated.

jmeyers314 commented 7 months ago

Thinking more about how to coerce .drawImage to use the X=1.0 "envelope" bandpass instead of the actual @image.bandpass when using the EnvelopeRatio photon operator: we could place that logic directly into LSST_SiliconBuilder. That'd make this work for our most important case I think, but errors might creep in if using a different stamp type.

Alternatively, we could refactor to require that image.bandpass be the X=1 envelope, and only build the X>1 actual bandpass when configuring LSST_SiliconBuilder. That'd have the downside though that image.bandpass is slightly a lie.

@rmjarvis , do you have an opinion here?

cwwalter commented 7 months ago

Sorry have been busy with first trip to summit after moving. @rmjarvis did you see Josh's question above?

rmjarvis commented 7 months ago

I thought you would just specify the config bandpass as being the one without the airmass factor and let the effective bandpass with the atmosphere be emergent from the combination of that with the EnvelopeRatio. But I admit I haven't thought about this enough to understand why that doesn't work for wavelength sampling as Josh implied above.

cwwalter commented 7 months ago

Hi Josh,

What's your current status on this and what do you still need help/input on?

I think we wanted to start the rehearsal production really soon so we should figure out how to do it:

You asked above for advice on how to:

Did you get any out of this issue?

jmeyers314 commented 7 months ago

Jim, Eli and I chatted a bit about this last Friday. I think we decided that instead of initially generating photons from the outer envelope bandpass and then probabilistically culling them to match the realized bandpass, we'd instead generate from the good 'ole X=1.2 throughputs and just modify photon fluxes (weights) to get to the appropriate bandpass. That's much easier than trying to coordinate envelope+realized bandpass. The downside is the Poisson statistics don't end up quite right, but since the airmass and QE variations are pretty small, I bet we can live with that. (And it's still better than what we had before).

I can probably deliver the above by ~tomorrow.

cwwalter commented 7 months ago

Intersting. So, the interpolation would return a weights table?

I thought I remembered the Galsim photon list can take a weight for entry, but can't find that now. Will GalSim itself apply the weight?

jmeyers314 commented 7 months ago

Okay, I think this one is ready to look at now. Tests pass for me locally (with branch tickets/DM-42513 of obs_lsst_data).

jmeyers314 commented 7 months ago

Okay, looks like the one CI failure is where it can't find the file that lives on an unmerged branch of obs_lsst_data. Good.

jmeyers314 commented 7 months ago

@jchiang87 , I think I've addressed all your comments now. Let me know if there's anything else I should look at.

@rmjarvis , could you take a peek at this, especially the tests to make sure they address some of the concerns you raised at the last imsim technical meeting?