Issue/5/realistic sky model #40
Conversation
| # But, we need a more realistic sky model and we need to pass more than | ||
| # this basic info to use Peter Y's ESO sky model. | ||
| # We must pass obs_metadata, chip information etc... | ||
| self.noise_and_background = ESOSkyModel(obs_metadata, addNoise=True, |
There was a problem hiding this comment.
I'm wondering of we should allow self.noise_and_background and self.PSF to be settable through the constructor, i.e.,
def imSim__init__(self, phosim_objects, obs_metadata, catalog_db=None, noise_and_background=None, PSF=None):
and set them to the values here if the default None values are passed.
There was a problem hiding this comment.
So, in practice it would show up here right:
phoSimStarCatalog = desc.imsim.ImSimStars(starDataBase, obs_md)
and would look something like
phoSimStarCatalog = desc.imsim.ImSimStars(starDataBase, obs_md, PSF=special_PSF)
This sounds OK except it seems to me that that PSF is not really a feature of the source and you will have separate entries like above for Galaxies etc. Perhaps it should be part of the common obs_metadata (which is already repeated)?
There was a problem hiding this comment.
I'm not sure what the right answer is. I might have expected PSF and noise_and_background to be class-level variables so that they would automatically consistent across object types. I'm happy to defer this question for now. If someone really wants to change it, they can do so explicitly for each instance by setting the attribute directly.
There was a problem hiding this comment.
OK, how about adding an issue for now to make these class level variables and we can do it separately in the future?
There was a problem hiding this comment.
sure, I'll make one, though that would imply changes to the sims code.
There was a problem hiding this comment.
Creating images with different object types is always going to be awkward because of the way objects are stored on fatboy (different tables for stars, galaxies, asteroids, etc.).
sims_GalSimInterface includes a method copyGalSimInterpreter() in GalSimCatalog which allows you to copy the self.galSimInterpreter (which actually does the image generation) from catalog to another to ensure that all of the objects land in the same FITS files. We could amend that method so that it also copies self.PSF and self.noise_and_background from one GalSimCatalog to another.
|
|
||
| def __init__(self, obs_metadata, seed=None, addNoise=True, addBackground=True): | ||
| """ | ||
| @param [in] addNoise is a boolean telling the wrapper whether or not |
There was a problem hiding this comment.
I know that this way of describing function arguments is what's used in the sims code, but for Twinkles and other DESC repos, we've decided that the numpy style docstrings are preferable. I don't have a super strong opinion and don't really care if we mix styles, but I do think that the numpy style is easier to read.
There was a problem hiding this comment.
Actually, we did make the decision (about a year ago) to adopt numpy style doctrings in the sims stack. We just haven't made the decision to budget the time to implement that decision, so all of our older code still has the '@param' format docstrings.
There was a problem hiding this comment.
Can you summarize the difference?
There was a problem hiding this comment.
This function is this way since I am deriving from a Sim's one that uses this function.
There was a problem hiding this comment.
Can you summarize the difference?
I think that the parameter names are easier to scan when reading the numpy style, plus the old sims way looks like it was based on Doxygen and so implies some C/C++-ism that aren't really appropriate for python code, e.g., function arguments should always be "[in]", i.e., assumed to be passed by value and not modified or set for output. There are numpy style examples in this repo, but I usually look at https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt for guidance.
There was a problem hiding this comment.
Ah, OK so that style is for DOxygen... I didn't know about the structured text documentation on the other style of docstrings. Thanks.
There was a problem hiding this comment.
Anyway, consistency is better. I'm not sure whether it is more consistent to be the same as the sims_interface (especially if it will change) or the rest of DESC code. Probably rest of DESC code. Feel free to change this now if you can do it quickly. Otherwise we can do it later. I think this code is probably going to be radically restructured when we make it work per pixel so I don't think leaving it now is terrible.
| sims_GalSimInterface/python/lsst/sims/GalSimInterface/galSimNoiseAndBackground.py | ||
| ''' | ||
|
|
||
| from __future__ import absolute_import, division |
There was a problem hiding this comment.
I added this line so that the python 3 checks in the travisc-ci builds would pass.
There was a problem hiding this comment.
OK thanks. I will try to remember this for the future.
|
I need to update the zero point values with the new information from Javier. I will do that in a bit. |
| effective_area=33.212*u.m**2, pixel_size=0.2*u.arcsec): | ||
| # Lookup the zero point corresponding to 24 mag/arcsec**2 | ||
| B0 = 24. | ||
| s0 = (dict(u=0.732, g=2.214, r=1.681, i=1.249, z=0.862, y=0.452)[filter_band] * |
There was a problem hiding this comment.
If these numbers may change, then perhaps they should be put in the configuration file, https://github.com/LSSTDESC/imSim/blob/master/data/default_imsim_configs, e.g.,
[skyModel_params]
B0 = 24.
u = 0.732
g = 2.214
r = 1.681
i = 1.249
z = 0.862
y = 0.452
There was a problem hiding this comment.
That might be good (how hard is it to add?).
In the longer term I wanted to change this to using the simulation project calls to return these numbers. Now, these come from David's external calculations. That's one of the things I was hoping to learn about on Thur's SSim meeting.
There was a problem hiding this comment.
it's straight-forward. I can do it, if you want.
There was a problem hiding this comment.
OK, please do. For right now it is better than hard coding it.
There was a problem hiding this comment.
According to @fjaviersanchez the new values are supposed to be:
zp_u = 0.282598538804 1 / (m2 s)
zp_g = 1.56492206349 1 / (m2 s)
zp_r = 1.3488785992 1 / (m2 s)
zp_i = 0.998783620839 1 / (m2 s)
zp_z = 0.699897205858 1 / (m2 s)
zp_y = 0.32665889564 1 / (m2 s)
|
OK, I looked at the latest changes and tried doing a run (see issue #5). I think other than the question of if the value is correct I think this is ready to merge. Do you agree Jim? |
|
yes, let's do it....I just hit the approval button. |
This pull request contains code to replace the exampleCCDNoise background class from the GalSim interface which uses a M5 level with @yoachim's ESO based sky model. Peter's model returns the sky level in mag/arcsec^2 and @dkirkby's function skyCountsPerSec converts that into a count level using the exposure time in the instance catalog stored in photParams.exptime. Note that David has calculated the zero points himself for a set of LSST filters. This should be changed to use the photUtils code.
Right now this just sets the whole skylevel based on the central pointing. I think as an intermediate step we want to this per chip, and then probably eventually build a healpix table completely outside of the loop so we can assign each pixel. But, I was thinking it might be useful to get this code in first and try it to confirm it gives reasonable signal/background levels and possibly put some options in for sky background choice.
Note there are also several small changes to meet PEP8 DM guidelines.