search

GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
228 stars 107 forks source link

write code to carry out standard multi-object simulations #103

Closed rmandelb closed 12 years ago

rmandelb commented 12 years ago

There are certain types of multi-object simulations that are standard in our field. Examples include ring tests, and the generation of multiple noise maps for the same object to determine error distributions. We would like to have some bits of code that can carry out these standard types of multi-object simulations automatically.

rmjarvis commented 12 years ago

I'll take this one. These will basically be a couple more demo scripts for the functionality that we want to have working for the next milestone. It probably requires quite a few other issues to be finished first, but like last time, working on this will probably help to figure out what issues we need to create.

rmandelb commented 12 years ago

Sounds good to me, thanks. I'm happy to chat about what kinds of functionality we want to include (beyond the 2 that I already listed above).

rmjarvis commented 12 years ago

I just pushed a first pass at a script for making Great08-like images. It currently only does 10 x 10 postage stamps rather than 100 x 100, since it is rather slow. Particularly the part that makes the star image. Shark says most of the time is spent calculating the sinc function. In particular the sin(x) function. So there might be some lookup-table optimization we could do there.

Anyway, the three issues that this script really want to be resolved are

rmjarvis commented 12 years ago

We can also talk about what other scripts we want for this milestone. Three is probably a good number again, so let's aim for that. Also, that's the number of output formats proposed in #102, so we can do a different one in each script.

The second script could be similar to the first but take its inputs from a configuration file, hence needing some resolution of #101. Also, we can include some different distributions as input parameters a la #98. I currently have the galaxy ellipticity distributed as a gaussian with a specified rms, but that's about it. It would be nice to have some distributions for the Sersic indices, bulge/disk ratios, etc.

The third script should probably usee Rachel's COSMOS galaxies (#104) and a real atmospheric PSF (#105). Anything else we want to include?

rmandelb commented 12 years ago

Thanks for making a start at this, Mike. A few comments -

As for the number of scripts, I think 3 is fine, however I could imagine it might be useful to set them up so that the user can put a command-line arg for the type of output they want (rather than having each script generate a different, specific type of output).

I think that the options you listed cover the issues in the milestone pretty thoroughly, so I don't have anything to add.

barnabytprowe commented 12 years ago

HI Mike,

Agree with 3 scripts, and wanting to get #65 out of the way. #71 should be quick if we're happy to stick to the previously agreed naming conventions for the time being and defer reopening this discussion until later.

Rachel, if you'd be happy to take a look at #65 I'd be happy to take you up on this offer because...

...It's very clear to me that the shape of these scripts must necessarily be being held back somewhat by what is currently still being developed in #101, i.e. the means of reading in a configuration file (which would set many of the things currently being set in the MultiObjectDemo.py script body here), and to a somewhat less urgent extent by #102. These are top priority items for me right now.

I've made some progress on #101 on the config file side, after learning a bit from Jim's example code, and would be happy to modify what you've currently got Mike to use it, in a day or so once there's a pull request made. But I'll keep pushing things there as they grow.

For a current idea of what's being done in #101 re. the config file, I'm currently reading in a default file using the eval() type functionality of Python. The default file is in a new directory config/. This is done via:

>>> import galsim
>>> config = galsim.config.load()

which makes a home-grown class object called an AttributeDict (thanks to Jim, although I made one mod), which in the default case contains

>>> config
noise.distribution = ['CCDNoise']
noise.gain = 1.0
noise.read_noise = 1.0
input_cat.type = 'ASCII'
input_cat.ascii_fields = ['x', 'y', 'nx', 'ny', 'Exponential.re', 'Exponential.e1', 'Exponential.e2', 'DeVaucouleurs.re', 'DeVaucouleurs.e1', 'DeVaucouleurs.e2', 'Moffat.FWHM', 'Moffat.truncationFWHM', 'Moffat.e1', 'Moffat.e2']
PSF.type = ['Moffat', 'Pixel']
PSF.Moffat.flux.distribution = 'Constant'
PSF.Moffat.flux.value = 1.0
PSF.Pixel.xw.distribution = 'Constant'
PSF.Pixel.xw.value = 1.0
PSF.Pixel.yw.distribution = 'Constant'
PSF.Pixel.yw.value = 1.0
user_config_file = None
galaxy.type = ['DeVaucouleurs', 'Exponential']
dx = 1.0
output.type = 'IMAGE'
logging_level = 20
shear.g2.distribution = 'Constant'
shear.g2.value = 0.0
shear.g1.distribution = 'Constant'
shear.g1.value = 0.0

(more stuff to be added / modified - I expect all of this to change A LOT as we iterate to what we find useful)

The idea is that users can submit their own config files for this sort of info - the default config file will get read first (or perhaps optionally even not at all) and so the users can reassign any values in the config object to be what they want.

Anyway, I'll try and get some of this pull request-ready ASAP so we can discuss it... And Mike, I suggest you continue as-is for the moment, I'll be able to modify the MultiObjectDemo.py script to use all this machinery post-hoc as required.

rmandelb commented 12 years ago

Yes, I'll take a look at #65 today / tonight.

rmandelb commented 12 years ago

Mike, I checked out the progress here after you made your new issue, and first I have to say that this is really cool.

I'm a little confused about the timing issue: if the galaxy images involve convolution of galaxy, moffat, pixel, and the stars are a convolution of moffat and pixel, then why is only the latter slow? If sinc is responsible then isn't that showing up in both?

rmjarvis commented 12 years ago

I suspect it has more to do with how many times sinc is called. SBProfile must be making a much larger k-space image for the stars than for galaxies. I guess the smoothness of the galaxy profile means that it knows that it doesn't need to go as big in k-space as it needs when there are the hard edges.

barnabytprowe commented 12 years ago

It could be related to this:

SBMoffat::SBMoffat(double beta_, double truncationFWHM, double flux_, double size, RadiusType rType) : 
        beta(beta_), flux(flux_), norm(1.), rD(1.),
        ft(Table<double,double>::spline)
    {
        //First, relation between FWHM and rD:
        FWHMrD = 2.* std::sqrt(std::pow(2., 1./beta)-1.);
        maxRrD = FWHMrD * truncationFWHM;
        // Make FFT's periodic at 4x truncation radius or 8x half-light radius:
        stepKrD = M_PI / (2*std::max(maxRrD, 16.));
        // And be sure to get at least 16 pts across FWHM when drawing:
        maxKrD = 16*M_PI / FWHMrD;

In particular, this line:

        // Make FFT's periodic at 4x truncation radius or 8x half-light radius:
        stepKrD = M_PI / (2*std::max(maxRrD, 16.));

from src/SBProfile. We've been setting big truncationFWHM values (which is actually an multiplying factor of the FWHM to use for the scale radius, confusingly, but which I note matches how they set the truncation in GREAT08), so this might be a problem. In GREAT08, using the definition above, truncationFWHM = 2 only. Might be worth trying. And might be worth renaming this parameter!!

rmjarvis commented 12 years ago

I actually dropped it back down to 2. It didn't make an appreciable difference though.

rmjarvis commented 12 years ago

On the speed that is. The images looked different. :)

rmandelb commented 12 years ago

I agree about renaming truncationFWHM. Or at least documenting it more thoroughly, for users who might not want to dig into the code to see what it does.

barnabytprowe commented 12 years ago

Maybe we could optionally drop the truncation radius altogether? It's the source of one of the two hard edges, and will require arbitrarily high resolution in k space to describe it properly: so not possible. I think that real space hard edges die off ~1/k in Fourier space, which is a terrible cost really...

barnabytprowe commented 12 years ago

Hmmm, sorry I keep posting on #103 not #141!

barnabytprowe commented 12 years ago

Hi all. Have merged #65 with this branch and updated base.py to include a working DeVaucouleurs, just for info.

barnabytprowe commented 12 years ago

Hi Mike, I just pushed an updated default catalog for reading in, with dx and dy, and removed the TODO. This, and all your changes on this branch, should be updated on #103 if you pull!

barnabytprowe commented 12 years ago

Hi Mike, I also noticed your bug fix which was actually my bug... the first arg of ReadInputCat should not be config, that can be deleted... (if you wish you can cherry pick the change from my branch)

rmjarvis commented 12 years ago

On May 9, 2012, at 9:32 PM, Barnaby Rowe wrote:

Hi Mike, I also noticed your bug fix which was actually my bug... the first arg of ReadInputCat should not be config, that can be deleted... (if you wish you can cherry pick the change from my branch)

Actually I think it should be. Eventually we want to get the file name from the config rather than requiring it to be supplied. So even though it's not used now, I think it's worth keeping that in there.

barnabytprowe commented 12 years ago

OK.

On 9 May 2012, at 19:26, Mike Jarvis wrote:

On May 9, 2012, at 9:32 PM, Barnaby Rowe wrote:

Hi Mike, I also noticed your bug fix which was actually my bug... the first arg of ReadInputCat should not be config, that can be deleted... (if you wish you can cherry pick the change from my branch)

Actually I think it should be. Eventually we want to get the file name from the config rather than requiring it to be supplied. So even though it's not used now, I think it's worth keeping that in there.

Reply to this email directly or view it on GitHub: https://github.com/GalSim-developers/GalSim/issues/103#issuecomment-5616785

Barnaby Rowe

Jet Propulsion Laboratory California Institute of Technology MS 169-237 4800 Oak Grove Drive Pasadena CA 91109 United States of America

Department of Physics & Astronomy University College London Gower Street London WC1E 6BT United Kingdom

barnabytprowe commented 12 years ago

Hi Mike...

Quick qustion: what framework should we choose to decide between the two BuildGSObject schemes we've got going in #101 and #103?

At some point before the weekend, one will need to be merged into the master for tagging as milestone 2. Both are preserved in the repo now for posterity anyway if we want to go back, and as either one will 100% likely need to change down the line I don't have a strong preference. An immediate example of this is the ability to handle shears and shifts - this is something that I'm not sure needs to necessarily be done in parallel.

My feeling is: I think this was a useful exercise and we now have a nice demonstration of two somewhat different approaches to skinning the same cat, and a great basis for discussing the tricky and open-ended problem of the user input interface to GalSim as a whole. Both seem to work too, which is nice, and each have pros and cons. Both are similar length and will take similar CPU time (v. small in both cases I think). Bu I think we should probably pick one today and focus on all the minor fiddly extra details to get it working.

(P.S. Did you also see my changes to the MultiObjectDemo.py script in #101? I think at least some of these will need to be adopted / modified to get things working.)

barnabytprowe commented 12 years ago

P.S. Also pinging @rmandelb for thoughts on the above? Shall we make an issue to decide, toss a coin, or poll around for opinions on this issue?

barnabytprowe commented 12 years ago

(P.P.S. My vote still goes for the one-stop object/parameter dictionary approach, but I'm only human :)

rmandelb commented 12 years ago

Uh, time for me to confess that since yesterday afternoon I've not been tracking the progress on your parallel approaches in #101 and #103, because I wanted to do a decent job at #104. Sorry. So I can't give a really informed opinion. My suggestion is this: write up a brief (few sentence) high-level description of each approach (which could probably be taken from the discussions on here) and maybe a few points explicitly comparing the two approaches -- this one has the advantage in X situation, the other is better for Y -- then ping a few people including me, Jim, etc. for a quick discussion. My motive here is not just to get the rest of us up to speed, but also because I sometimes find that making such a high-level description and comparison clarifies matters for myself, so you and Mike might also get some benefit out of the exercise and out of stepping back from coding for a short time. But if this is too much make-work that you don't think would be helpful, then never mind.

(But I do agree that we should probably pick one and do the fiddly details only on it, rather than on both. And I'll stop being lazy and go look at the code on those branches this afternoon.)

rmjarvis commented 12 years ago

I figured we'd ask Jim and/or Joe for their opinion, as I think they are both sort of python guru-ish. I'm pretty happy with either approach. Of course I still think mine is cleaner, especially with regard to the ease of extensibility for more complicated Generate functions. But I'm happy to defer to outside opinions for a decision and just use that going forward.

barnabytprowe commented 12 years ago

I think the discussion on #101 is the best place to start for a summary of the different approaches, a bit more than mid way down I think. The Python gets a little bit obfuscatory at times since we're doing so much "meta"-level stuff...

On 10 May 2012, at 10:33, Rachel Mandelbaum wrote:

Uh, time for me to confess that since yesterday afternoon I've not been tracking the progress on your parallel approaches in #101 and #103, because I wanted to do a decent job at #104. Sorry. So I can't give a really informed opinion. My suggestion is this: write up a brief (few sentence) high-level description of each approach (which could probably be taken from the discussions on here) and maybe a few points explicitly comparing the two approaches -- this one has the advantage in X situation, the other is better for Y -- then ping a few people including me, Jim, etc. for a quick discussion. My motive here is not just to get the rest of us up to speed, but also because I sometimes find that making such a high-level description and comparison clarifies matters for myself, so you and Mike might also get some benefit out of the exercise and out of stepping back from coding for a short time. But if this is too much make-work that you don't think would be helpful, then never mind.

(But I do agree that we should probably pick one and do the fiddly details only on it, rather than on both. And I'll stop being lazy and go look at the code on those branches this afternoon.)

Reply to this email directly or view it on GitHub: https://github.com/GalSim-developers/GalSim/issues/103#issuecomment-5631395

Barnaby Rowe

Jet Propulsion Laboratory California Institute of Technology MS 169-237 4800 Oak Grove Drive Pasadena CA 91109 United States of America

Department of Physics & Astronomy University College London Gower Street London WC1E 6BT United Kingdom

rmandelb commented 12 years ago

Okay, I'll go there in a bit. And I agree w/ Mike that Jim and Joe would be good people to ping.

On May 10, 2012, at 1:35 PM, Barnaby Rowe wrote:

I think the discussion on #101 is the best place to start for a summary of the different approaches, a bit more than mid way down I think. The Python gets a little bit obfuscatory at times since we're doing so much "meta"-level stuff...

On 10 May 2012, at 10:33, Rachel Mandelbaum wrote:

Uh, time for me to confess that since yesterday afternoon I've not been tracking the progress on your parallel approaches in #101 and #103, because I wanted to do a decent job at #104. Sorry. So I can't give a really informed opinion. My suggestion is this: write up a brief (few sentence) high-level description of each approach (which could probably be taken from the discussions on here) and maybe a few points explicitly comparing the two approaches -- this one has the advantage in X situation, the other is better for Y -- then ping a few people including me, Jim, etc. for a quick discussion. My motive here is not just to get the rest of us up to speed, but also because I sometimes find that making such a high-level description and comparison clarifies matters for myself, so you and Mike might also get some benefit out of the exercise and out of stepping back from coding for a short time. But if this is too much make-work that you don't think would be helpful, then nev er mind.

(But I do agree that we should probably pick one and do the fiddly details only on it, rather than on both. And I'll stop being lazy and go look at the code on those branches this afternoon.)

Reply to this email directly or view it on GitHub: https://github.com/GalSim-developers/GalSim/issues/103#issuecomment-5631395

Barnaby Rowe

Jet Propulsion Laboratory California Institute of Technology MS 169-237 4800 Oak Grove Drive Pasadena CA 91109 United States of America

Department of Physics & Astronomy University College London Gower Street London WC1E 6BT United Kingdom

Reply to this email directly or view it on GitHub: https://github.com/GalSim-developers/GalSim/issues/103#issuecomment-5631467

Rachel Mandelbaum http://www.astro.princeton.edu/~rmandelb rmandelb@astro.princeton.edu

barnabytprowe commented 12 years ago

OK, I'm going to make a quick issue describing the question. Then Mike, you and I will add a paragraph describing our solution, perhaps with a link to the github view into the relevant branch's frontend.py

When both our methods are described, we'll ping the Python gurus. Sound good?

rmjarvis commented 12 years ago

Check.

barnabytprowe commented 12 years ago

OK Mike, this is a new issue #143. I don't think it matters who posts first about the approach, so I'll start preparing my short blurb now.

rmjarvis commented 12 years ago

Scripts 1 and 2 are now both working. (Using merges from #102 and partially #101.)

I haven't made a start on script 3 yet, which is to showcase Rachel's real galaxies. And I won't have much of a chance to work on this for the rest of the day.

So Rachel, if you feel like taking ownership of this to work on in parallel with your work on #104, that's fine with me. Especially if you're having trouble coming up with unit tests to do, it might be helpful to at least test the code by running a script on it. That at least tends to catch syntax errors and such.

rmandelb commented 12 years ago

Just to quickly close the loop on this:

in #104 I have a quick, single-object demo script that allowed me to do basic debugging and sanity checking. I don't have time to work on a multi-object script, unfortunately... see details over there.

On May 10, 2012, at 4:08 PM, Mike Jarvis wrote:

Scripts 1 and 2 are now both working. (Using merges from #102 and partially #101.)

I haven't made a start on script 3 yet, which is to showcase Rachel's real galaxies. And I won't have much of a chance to work on this for the rest of the day.

So Rachel, if you feel like taking ownership of this to work on in parallel with your work on #104, that's fine with me. Especially if you're having trouble coming up with unit tests to do, it might be helpful to at least test the code by running a script on it. That at least tends to catch syntax errors and such.

Reply to this email directly or view it on GitHub: https://github.com/GalSim-developers/GalSim/issues/103#issuecomment-5635247

Rachel Mandelbaum http://www.astro.princeton.edu/~rmandelb rmandelb@astro.princeton.edu

rmjarvis commented 12 years ago

Barney, I actually did manage to finish my alternate config style where all the different attributes can be specified as strings. It's all the same things as we've been specifying in the config object but just combining multiple lines into one.

Here is the old way:

        config.psf.type = 'Moffat'
        config.psf.beta.type = 'InputCatalog'
        config.psf.beta.col = 5
        config.psf.fwhm.type = 'InputCatalog'
        config.psf.fwhm.col = 6
        config.psf.ellip.type = 'E1E2'
        config.psf.ellip.e1.type = 'InputCatalog'
        config.psf.ellip.e1.col = 7
        config.psf.ellip.e2.type = 'InputCatalog'
        config.psf.ellip.e2.col = 8
        config.psf.truncationFWHM.type = 'InputCatalog'
        config.psf.truncationFWHM.col = 9

        config.pix.type = 'SquarePixel'
        config.pix.size = pixel_scale

        config.gal.type = 'Sum'
        config.gal.items = [galsim.Config(), galsim.Config()]
        config.gal.items[0].type = 'Exponential'
        config.gal.items[0].half_light_radius.type = 'InputCatalog'
        config.gal.items[0].half_light_radius.col = 10
        config.gal.items[0].ellip.type = 'E1E2'
        config.gal.items[0].ellip.e1.type = 'InputCatalog'
        config.gal.items[0].ellip.e1.col = 11
        config.gal.items[0].ellip.e2.type = 'InputCatalog'
        config.gal.items[0].ellip.e2.col = 12
        config.gal.items[0].flux = 0.6
        config.gal.items[1].type = 'DeVaucouleurs'
        config.gal.items[1].half_light_radius.type = 'InputCatalog'
        config.gal.items[1].half_light_radius.col = 13
        config.gal.items[1].ellip.type = 'E1E2'
        config.gal.items[1].ellip.e1.type = 'InputCatalog'
        config.gal.items[1].ellip.e1.col = 14
        config.gal.items[1].ellip.e2.type = 'InputCatalog'
        config.gal.items[1].ellip.e2.col = 15
        config.gal.items[1].flux = 0.4
        config.gal.flux = gal_flux
        config.gal.shift.type = 'DXDY'
        config.gal.shift.dx.type = 'InputCatalog'
        config.gal.shift.dx.col = 16
        config.gal.shift.dy.type = 'InputCatalog'
        config.gal.shift.dy.col = 17
        config.gal.shear.type = 'G1G2'
        config.gal.shear.g1 = gal_g1
        config.gal.shear.g2 = gal_g2

And the equivalent thing using the new style to the maximum extent possible is:

        config.psf.type = 'Moffat beta=<InputCatalog col=5> fwhm=<InputCatalog col=6> ' +\
                'truncationFWHM=<InputCatalog col=9> '
        config.psf.ellip = 'E1E2 e1=<InputCatalog col=7> e2 = <InputCatalog col = 8>'

        config.pix = 'SquarePixel size=%f'%pixel_scale

        config.gal.type = 'Sum items=[ ' +\
                'Exponential half_light_radius=<InputCatalog col=10> ' +\
                    'ellip=<E1E2 e1=<InputCatalog col=11> e2=<InputCatalog col=12> > ' +\
                    'flux = 0.6 ,' +\
                'DeVaucouleurs  half_light_radius=<InputCatalog col=13> ' +\
                    'ellip=<E1E2 e1=<InputCatalog col=14> e2=<InputCatalog col=15> > ' +\
                    'flux = 0.4 ]'
        config.gal.flux = gal_flux
        config.gal.shift = 'DXDY dx=<InputCatalog col=16> dy=<InputCatalog col=17>'
        config.gal.shear = 'G1G2 g1=%f g2=%f'%(gal_g1,gal_g2)

Well, not quite the maximum possible. But a lot.

Sometimes this seems like an obvious win. Like: config.gal.shift being one line rather than 5 to say the same information. But config.gal is a bit dense having nesting inside nesting inside an array. But I just went for the maximum amount of info being put into the new style to stress test the code as much as possible. In practice you can mix and match the two style in any way you want.

I checked that the output from the two config objects produce the same cube.fits file in the end, so I don't think I've made any egregious errors at least. So let me know what you think. If you like it, we can merge it over to 101 and include it in the pull request. If you think it needs more discussion, so put off to the next milestone, or should be scrapped entirely, that's fine too.

rmandelb commented 12 years ago

Hmmm, I do like it better for shift, but as you say, the galaxy is a bit messy. For myself I'd probably adopt a mixed style. But -- could we perhaps discuss more? I don't think it needs to be put off to the next milestone in a month -- we could aim to decide on the 1 week timescale (for demo script 3 + real galaxies + Kolmogorov).

rmandelb commented 12 years ago

Now that we're close to merging in #104 (I hope), I wanted to touch base about the final demo script. In particular:

1) Mike, did you already have a plan for this that you plan to carry out or 2) Do you want me to come up with something?

rmjarvis commented 12 years ago

I don't mind doing it, but I haven't thought about it at all other than to do 100 of your real galaxies in some form. if you want to try to flesh that out a bit, feel free.

rmandelb commented 12 years ago

Yeah, that's about as far as I got in thinking about it. 100 galaxies goes nicely on a 10x10 grid... :) As for who does it, I really have no preference. My intent is that this should be a very simple variation on scripts 2 and 3 using a lot of the same code just changed to make real galaxies -- something that shouldn't take much time for either of us and that would show off this particular aspect of the code in a way that users will find easy to understand. If we want Kolmogorov PSFs then we might have to wait a bit, as I see Joerg has started a branch but I don't know how soon to expect it to get finished and merged in.

rmandelb commented 12 years ago

Just FYI so we don't duplicate each other's work, I'm going to merge master into this branch and make a start at script 3. Will have to hand it off in ~30 minutes to attend to other things, though.

rmandelb commented 12 years ago

Hmmm, I just realized that this branch is a bit ahead of master with some other developments/improvements, so I'm not sure it makes sense to work on the demo script on here (unless we're sure we'll wnat to merge that into master at the same time that we will want to merge the other new things).

rmjarvis commented 12 years ago

The other new stuff in that branch is using strings to abbreviate the config specifications.

rmandelb commented 12 years ago

Okay, well, I went ahead in #103 and we can always put the changes elsewhere if that turns out to be better.

I can't work on this for the rest of the day, but what I have so far is:

1) creation of a double Gaussian PSF (eventually we want Kolmogorov but I do't know when that will be ready) 2) a loop over the galaxies in the real galaxy catalog, where for each of them we deconvolve, randomly rotate, shear by the same shear in each case, convolve with the same PSF in each case, resample to the same target pixel scale (using simReal for basically everything``` 3) It writes to a cube and to a multi-HDU file, just like script 2. It might be nice to do something different here, but I ran out of time.

I had a bit of (admittedly nerdy) fun opening the cube in DS9 and making a slide show of the sims based on the real galaxies. It was also a good learning experience, since when you do this on a linear scale it looks good, but on a log scale you can see artifacts that I think might be related to zero-padding (but, admittedly, I didn't have any time to investigate, and won't be able to do so today).

I tried to put in noise similarly as in script 2 but the images don't look noisy. This must be a bug but again, I don't have time to track it down.

I can work on this more on the weekend, or if someone else wants to take over, that's fine. I don't normally like to push change that clearly have some bugs, but I didn't want to leave this hanging.

rmjarvis commented 12 years ago

I tried to put in noise similarly as in script 2 but the images don't look noisy. This must be a bug but again, I don't have time to track it down.

The noise level on these looks right. You used a sky level of 1.e6, and the outer pixels seem to range around +- 1.e3. So that's fine. I'm not sure if the flux on the galaxies is right. You have that set at 1.e6, but that's close to the level of the central pixel. And obviously there are a lot of pixels, so the total flux must be quite a bit more than that.

OTOH, the pixel scale is only 0.15, so maybe that explains it. Is the total flux supposed to be Sum I_ij * dx * dy? Or Sum I_ij?

From my recent look at the photon shooting stuff, where I originally got the flux wrong, I think those are also normalized to the former definition. But isn't that wrong? The total flux is just distributed among the pixels. The photons don't know about dx and dy. So shouldn't the latter definition be the one we want?

rmjarvis commented 12 years ago

And continuing on that theme, I think the way we do the sky level is probably also wrong. The sky really is in flux per square arcsec. It does know about dx, dy. So I think we should be adding sky_level * (dx*dy) to calculate the photon noise.

rmandelb commented 12 years ago

Gary's definition is that these are surface brightness profiles, so total flux = Sum I_ij * dx * dy. I am used to working with the other definition myself.

Good point about the sky, I think we're currently being inconsistent.

rmjarvis commented 12 years ago

Gary's definition is that these are surface brightness profiles, so total flux = Sum I_ij * dx * dy. I am used to working with the other definition myself.

I guess the question is what do we want to happen to the image when we change the pixel scale? (And let's bring in @gbernstein to get his thoughts on this.)

I think the total S/N of the image should stay the same. (Assuming the number of pixels changes to keep the same extent as measured in arcsec.) In the absence of read noise, the S/N shouldn't depend on the pixel scale, right?

So given a sky level in photons/arcsec^2, we should be adding sky_level * (dx*dy) to each pixel to calculate the poisson noise in each pixel.

And the galaxy's (or star's) values should scale by dx*dy as well to keep Sum I_ij constant.

It is probably a fair amount of work to make sure this change goes through correctly in the various draw commands, so if we do want to make this switch it should be on a different issue/branch. But do people agree with me that this makes more sense? Or am I missing something?

rmandelb commented 12 years ago

Can we perhaps separate this into 2 points:

1) how can we fix things for this issue with a minimum of fuss (perhaps by changing how values are defined in the demo scripts), and 2) what should our convention be in general?

I propose that we should discuss (1) here just to get the demo scripts working and making reasonable-looking images, and start a new issue for (2)... my motivation being, in part, that it's a significant point, and making a new issue will send an e-mail to everyone in the repo so they have a chance to comment if they wish.

rmandelb commented 12 years ago

And in the spirit of my message above, my suggestion for this particular issue is that

(a) we should decide if there's anything else of interest that we want this script to do. Since Kolmogorov profiles are not in yet, and we don't want to drag out these demo scripts indefinitely (would like to merge this in, let the group know that there are more things to play with, make a tag for the 2nd milestone, etc.), my proposal is to move forward with the double Gaussians that are currently in there. Are there any other changes that you'd be interested in seeing? It does the same output formats as script 2 right now, which I kind of like because of how DS9 displays the FITS data cube.

(b) we have to decide if we want something noisier for script 3. I might adjust the noise level upwards a little, but not too much.

(c) we have to decide if we'll want to merge in the abbreviated config specifications, or if I should move the work in demo script 3 to a separate branch that doesn't include it. I personally like the abbreviated stuff in some cases but not others, but since it's optional, I'd be happy to include it in a pull request with the demo script.

barnabytprowe commented 12 years ago

Definitely a good idea to move the flux = Sum I_ij to a separate issue - this is a big one. I'll reserve my comments on the matter for that Issue, thanks for opening that Mike.

Turning to Rachel's abc...

a) Agree with keeping to Double Gaussians. I also really like the DS9 datacube slider functionality (I've been using it for years!)

b) I think we want to make these galaxies look somewhat realistic for this demonstration of functionality, but am not too worried about much more.

c) I apologise for not making comments on this sooner Mike. I certainly don't think the abbreviated/string-interpreting config system should be scrapped: I personally don't have strong objections to there being more than one way to provide input to the code, and I think this method is undeniably convenient in some cases (while perhaps a little obfuscatory in others, where nesting was involved).

Rather rashly, I think that if the abbreviated config spec system is working we should just merge it with this branch, being clear that this is coming along with the main demo script. Then we raise an issue to discuss and get a reasoned consensus on it. It will need to be dicussed in tandem with Jim's work on #148, for example....

rmjarvis commented 12 years ago

if the abbreviated config spec system is working

As far as I'm concerned it is working. That's actually the reason I went for more nesting than I think is really preferable in the demo script -- to prove to myself that I coded up the nesting stuff right.

But perhaps I should write a version that tries to hit the sweet spot of optimal readability (since I think the fully explicit style is a bit eye-glazing in its repetition). Then we can get rid of the if True: line and just have a single example.

rmjarvis commented 12 years ago

But perhaps I should write a version that tries to hit the sweet spot of optimal readability (since I think the fully explicit style is a bit eye-glazing in its repetition).

I forgot to mention that I did this for script 2. I added a bunch of comments along the way to kind of walk people through some of the capabilities of the config structure. So if you're interested, please check it out and see what you think.