galsim-developers / galsim Goto Github PK

Jim, sorry to raise another Image class issue! :)

According to the ImageSIFD class docstrings, Image should have an (ncols, nrows) initialiser call, but it doesn't seem to be working…

If I try

In [31]: import galsim

In [32]: img = galsim.ImageS(5,5)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
/Users/browe/great3/GalSim/tests/<ipython-input-32-e631d2e352e5> in <module>()
----> 1 img = galsim.ImageS(5,5)

TypeError: numpy.ndarray argument required

From the ImageSIFD docstrings I was led to believe...

There are several ways to construct an Image:
Image(ncol, nrol)              # zero-filled image with origin (1,1)
Image(bounds=BoundsI(), initValue=0) # bounding box and initial value
Image(array, xMin=1, yMin=1)  # NumPy array and origin

Indeed, I've not been using any of these, just e.g. ImageD(array). Is this a known problem (in which case I will edit the docstring to add that some of these inititialization methods will come later), or is this something I should debug in the Python wrappers? Just wanted to clarify before diving in...

Thanks for your help!

We need to remove the NR random number generator (which Gary modified significantly) that is part of SBProfile. The current plan is to replace it with Boost random, but we should make sure this is done in a sufficiently general way that it is easy for users to switch between different number generators.

For the unit testing framework, we want to try making Python tests super easy to write via Nose:

http://readthedocs.org/docs/nose/en/latest/

The testing in python then amounts to changing directory to tests/ and typing:

nosetests

And we can also get useful coverage reports etc. I've learned that Nose is also a dependency for numpy, so it should be installed anyway. This solves the dependency issue. However, it might be that only the libraries are installed rather than the nosetests executable/script...

Mike, I've assigned you to this more as a question than anything else: can we test for the presence of the nosetests executable from SCons?

If so, could we add a line for that to the build and close the issue? Thanks!

The title just about says it all ^^. This should be a fairly simple change, which can be done as soon as #12 is merged to master.

We need a unit testing system in both C++ and python.

PYTHON

While there is a built-in unit testing module "unittesting", the "nose" framework, which is used by numpy among others, makes it almost trivially easy to write and run tests, so I would recommend that we use that. As a nice bonus it can provide coverage statistics showing which functions and methods have been tested and which have not.

C++
There seem to be several sensible options for C++ unit testing (based in responses on StackOverflow and the wikipedia list!):

• boost test
• UnitTest++
• cppunit

None of the three seems to have very nice documentation!

Boost has the advantage that we are already using part of it, so requiring it is not to much more of a stretch. It is also extremely simple to write tests.
UnitTest++ and cppunit seem to be a little more complex to get set up and running with tests, though possibly this is because they are more organized in the long run.

All three can produce the same XML-based output which can be imported into Jenkins, a web app I've asked Dugan at UCL to set up that can monitor the github repo and provide statistics about passing and failing tests in a continuous and graphical way.

Based on this I'd suggest using the boost system, but would welcome input.

We require unit tests to make sure that the random number generator that we are using is behaving the way we expect for a given initial seed (follow up to issues #30 and #31).

Assigned: Joerg, Barney, Rachel, Mike

Create an image of a very simple PSF that approximates the effects due to the atmosphere (e.g. a simple parametric Gaussian, Kolmogorov - I think Kaiser 2000 has some good material for this). This could be circular to begin with, or elliptical if we want to be ambitious. This PSF will convolved together with the PSF from the optics module to provide the overall PSF (pixelation not included) for the GalSim General Milestone 1 images.

I've assigned myself this issue, which is a straight port of some code I have for making aberrated optical PSF models in a simple (low order Zernike polynomial) kinda way. Will name the branch after this issue, which is also assigned to a "project milestone" -- I wanted to see what these were and whether they were helpful. Seems like a way of filtering issues basically, might be useful...

As people trial builds, issues will arise. Where there is an informative/useful solution, we should put this in the Install FAQ on the Wiki.

We need a python wrapper for the RNG. This will be used as input for the noise module for the first project milestone.

Barney is assigned, with Jim providing support / advise.

Hi Jim -
I put some test data into the repo and tried to run Shera.py (note - the exact command to call it and have it find the data is given in a comment at the top of the file). There were a few minor typos and syntax errors that I fixed and pushed, but I've hit a boost error that I can't understand:

Traceback (most recent call last):
File "Shera.py", line 69, in
main(sys.argv)
File "Shera.py", line 46, in main
psf1 = galsim.SBPixel(psf1Img, l32d, dxHST, 2.)
Boost.Python.ArgumentError: Python argument types in
SBPixel.init(SBPixel, ImageD, InterpolantXY, float, float)
did not match C++ signature:
init(_object*, galsim::Image image, galsim::Interpolant2d i, double dx=0.0, double padFactor=0.0)
init(_object*, int nPix, double dx, galsim::Interpolant2d i, int nImages=1)

So, it is claiming a mismatch between the python and the C++ arguments needed to create an SBPixel. The problem is that the set of arguments for the python does look like the first possible set of C++ arguments to the constructor to me, so I don't know why it's complaining. Can it not deal with a float vs. double mismatch? That appears to be the only issue, unless I'm completely missing something.

Jim said he could copy this over from LSST land. If you could also modify it to include our convention for putting the issue in the branch name, that would be good.

SBprofile requires DOxygen-style documentation. While we agreed at the telecon that people who write code should be the ones to document it, Barney and I propose that the pre-existing C++ code for SBprofile should be an exception to the rule. There are two reasons: (a) Gary already provided some documentation that isn't DOxygen-style, and it would be a waste of his limited time to require him to convert its format given his interest in being involved in more substantive things (SHERA vs. SBProfile comparison and the extension of sbprofile to include a photon-shooting method), and (b) Barney needs to become familiar with SBProfile and needs to learn DOxygen, so this job is a good way to get started on both of those tasks.

Currently the SBExponential profile expects to be given r0, whereas SBSersic wants an re value. This seems possibly confusing, as it means that SBSersic with n=1 would want a different number from what SBExponential uses to describe the same profile. Should we consider a change, or allowing a choice of r0 OR re specification for SBExponential?

I can see that for a disk we might want users to work in terms of disk scale length rather than half-light radius. I just don't think it should be the only choice.

There are other cases we might want to consider, e.g., for a Gaussian we currently need to specify sigma, which is the most natural thing to specify from a mathematical perspective, but sometimes if we're using a Gaussian as a PSF we might more naturally think of its FWHM; or, when making a Gaussian galaxy, we might care about its half-light radius.

So, the purpose of this issue is that we should give some thought to the most reasonable and least confusing way of specifying different types of radii for the various SBProfiles. It may be that we'll end up sticking with what we have now, but we should discuss.

(Can happen after first milestone - is not THAT urgent - but should happen before we write lots more code.)

Right now DOxygen complains because the boost random files that Gary added to resolve #1 aren't Doxygenized. Is there some way to tell Doxygen to skip them?

While the optics module is quite functional already, I see a note that says "implement a centrally-obstructed pupil plane (e.g. such as is caused by secondary mirrors)". This probably deserves its own issue, though I don't think it's needed for the first milestone.

It seemed to me that the Python docstrings for the wrapped SBProfile objects could be added to, and given my recent documentation of the SBProfile class in C++ it seems I'd be a good person to take this on. This shouldn't break code, so I won't create a separate branch. I might ping @TallJimbo from time to time with a question / issue!

Jim wants to make some changes to how we write Doxygen, so he needs a branch. That branch needs an issue.

Table.cpp in SBProfile uses a modified version of the NR spline. Per Gary's description, "The spline implementation is completely hidden inside the Table<> class so it can be rewritten with no effect on code outside this class." Best option might be to rewrite it ourselves using the rather simple definition of a spline (downside: significant testing required). Otherwise, is there some other open source code that we could use to replace this?

Mike made a file devutils/c.vim to enforce various aspects of the coding style that we agreed on, for those who use vim as their editor. It would be helpful if somebody who is an emacs ninja could do the same for emacs.

Gary and I need to finish our detailed comparisons of the IDL shera vs. sbprofile, so we understand whether there are real differences and where they might be arising.

I've assigned this to me, but really it's Gary and me (I wonder why github doesn't let us assign issues to two people??), and will begin with "the delta function test" -- i.e., taking the "galaxy image" to be equal to the HST PSF image, and making sure that after PSF-matching, the result simply equals the target PSF.

SBProfile has certain file and class naming conventions. It's slightly annoying that the hsm code is not consistent with that, so I need to change it.

Currently the adaptive moment and PSf correction code in src/meas_shape_hsm/ has a Makefile. It needs to be integrated into the existing build system. Rachel, as a complete scons newbie, is going to be so presumptuous as to assign this to Mike (but feel free to decline if you prefer not!). It's not a huge rush, but ideally would be done on the ~1 week time scale so we can play with images from sbprofile and test their moments compared to SHERA outputs.

As per Gary's suggestion on the mailing list, we need to collect our wisdom on how to install and build in one place (especially w.r.t. Python and Boost on Mac OS).

Because the possibility of work-duplication is even greater for documentation, if you plan to put in a lot of work on this, please assign the issue to yourself so others can see that you're working on it. Then clear the assignment if you've done your bit but you still think there's work to be done.

Mike, I noticed that the integ package does not have DOxygen-style comments. I see that it has many classes and functions so it would be a big job, but I think that it would be good to at least have DOxygen comments for the functions that we're actively using within GalSim, or expect to use in the near future. This looks like it would be a much more manageable task. Are you up for this? Or should Barney or I take this job on?

Not super time-critical, obviously.

On my 64-bit macbook pro with enthought python and the system c++ I tried to run with -pedantic, just to see what would happen (I don't think we necessarily should aim to build with this - I was just curious). I get:

joe$ scons EXTRA_FLAGS=-pedantic
scons: Reading SConscript files ...
SCons is version 2.0.1 using python version 2.7.2
Python is from /Library/Frameworks/EPD64.framework/Versions/7.2/include/python2.7
Using compiler: /usr/bin/g++
compiler version: 4.2
Determined that a good number of jobs = 4
<snip bunch of successful things)
Checking if we can build against Python... no
Cannot run program built with Python.

Which is fair enough. But then if I try to run again without that flag I once again get identical output. I have to manually remove the .scon* files before I can continue, or possibly rerun having specified EXTRA_FLAGS=" ". I am not sure if the latter works - sometimes it seems to work but sometimes not! I will try to clarify this.

This is also true if I try to clean with "scons -c".

Incidentally, can we stop scons from searching for all the dependencies if all we are going to do is clean with -c? It seems unnecessary. I seem to remember doing that in a hackish way in a previous scons script just by checking if "-c" was in sys.argv.

If TMV.h is found, the install can still fail to find tmv-link. Should look in ../share relative to TMV.h location.

Redhat Enterprise Linux (and derived distributions like Fedora) install libcfitsio.a in /usr/lib/ and /usr/lib64, but place fitsio.h in /usr/include/cfitsio/. The current build system expects a common root directory for include/fitsio.h and lib/libcfitsio.a.

Should scons be more flexible about this or should we just ignore this since we want to get rid of cfitsio anyway?

I want to put the top level class interface into a branch for discussion via pull request, this is the issue for that branch.

We need a few examples / a very simple demo for the new Image class, possibly included in the documentation itself. Jim, sorry if this is already there and I missed it...

Also include recommendations for best practices, style etc.

e.g. I prefer dox comments in the header files, rather than the cpp files, so when you are looking at the code for how to use a class, you also see the documentation directly without having to go to the separate dox pages or look into the .cpp files.

I think Joe volunteered for this on the telecon.

Assigned: Mike, Barney, Rachel, Peter, Joe, Bob, Mandeep

We want to have a simple Python user interface for generating the GalSim galaxy images, and this should be exemplified in some demo scripts to showcase the code in action. This will be a group effort, with advice and opinion warmly welcomed.

Mike, I added your name as the GitHub assignee because you are listed on both user interface and Demo scripts in our assignment documents, and I thought you would have some valuable ideas about the forms these should take! Python is super easy to pick up quickly (I managed it, albeit of course imperfectly), and most importantly we want our interface to also be super easy; I think having a relative newcomer to Python also working on the front end might actually help keep things obvious.

But I see this as being very much a group effort, including discussion and thought about the user interface.

Now that we have our new Image class (#12), we will want to make a python interface to the hsm code using the Image class, and eliminate the cfitsio dependency from GalSim entirely.

I've tentatively assigned this to myself, as I am the one who knows the hsm code best, but I have a lot to learn about making a python interface and will probably be pinging Barney and Jim for help. Should be a good learning experience (and won't hold up our achieving the first project milestone if I'm slow).

$ scons EXTRA_INCLUDE_PATH=/usr/include/cfitsio TMV_
DIR=$HOME
scons: Reading SConscript files ...
SCons is version 2.0.1 using python version 2.7.1
Python is from /usr/local/epd-7.0-1-rh5-x86_64/include/python2.7

Using compiler: g++
compiler version: 4.1.2
Determined that a good number of jobs = 8
Checking for C++ library cfitsio... yes
Checking for C++ library fftw3... yes
Checking for C++ header file boost/shared_ptr.hpp... yes
Checking for C++ header file TMV.h... yes
Using TMV_LINK file: /afs/umich.edu/user/j/o/jorgd/share/tmv-link
-L/afs/umich.edu/user/j/o/jorgd/lib -ltmv -lblas -lpthread -fopenmp
Checking for correct TMV linkage... (this may take a little while)
Checking for correct TMV linkage... yes
.sconf_temp/conftest_6: error while loading shared libraries: libpython2.7.so.1.
0: cannot open shared object file: No such file or directory
NameError: global name 'conf' is not defined:
File "/afs/umich.edu/user/j/o/jorgd/src/GalSim/SConstruct", line 955:
SConscript(script_files, exports='env')
File "/usr/local/epd-7.0-1-rh5-x86_64/lib/python2.7/site-packages/SCons/Script
/SConscript.py", line 614:
return method(_args, *_kw)
File "/usr/local/epd-7.0-1-rh5-x86_64/lib/python2.7/site-packages/SCons/Script
/SConscript.py", line 551:
return _SConscript(self.fs, _files, *_subst_kw)
File "/usr/local/epd-7.0-1-rh5-x86_64/lib/python2.7/site-packages/SCons/Script
/SConscript.py", line 260:
exec file in call_stack[-1].globals
File "/afs/umich.edu/user/j/o/jorgd/src/GalSim/pysrc/SConscript", line 12:
DoPythonConfig(pyenv)
File "/afs/umich.edu/user/j/o/jorgd/src/GalSim/SConstruct", line 903:
config.CheckPython()
File "/usr/local/epd-7.0-1-rh5-x86_64/lib/python2.7/site-packages/SCons/SConf.
py", line 640:
ret = self.test(context, _args, *_kw)
File "/afs/umich.edu/user/j/o/jorgd/src/GalSim/SConstruct", line 579:
if opt not in conf.env["LDFLAGS"]:
Checking if we can build against Python...

Setting LD_LIBRARY_PATH fixed the build problem, but scons should probably exit more gracefully.

I tried to go through the examples on https://github.com/GalSim-developers/GalSim/wiki/Examples:-getting-started-with-SBprofile-and-hsm and found that whenever I convolve a sheared profile with a box, the output image is blank. So

python SBDraw.py "sersic 2 2 S 0.2 0.2" test1.fits 0.2 works
python SBDraw.py "(sersic 2 2 S 0.2 0.2) * box 0.2" test2.fits 0.2 creates a blank image but
python SBDraw.py "(sersic 2 2) * box 0.2" test2.fits 0.2
python SBDraw.py "(gauss 0.4) * box 0.2" test3.fits 0.2 works
python SBDraw.py "(gauss 0.4 S 0.2 0.2) * box 0.2" test3.fits 0.2 creates a blank image

As an aside: I think
python SBDraw.py "(sersic 2 2 S 0.2 0.2) * (gauss 0.4)" test3.fits 0.2
should not complain about unmatched parentheses.

PyFITS should now be considered an optional dependency of GalSim. There is some code in the galsim Python module that uses it, and the SBDraw example requires it.

I have put the import pyfits statements at function scope, so you can still import galsim and use everything else even if you don't have PyFITS, but we should at least update our install documentation to include it.

We may also want to have SCons test for its presence. For now, this would only be useful for providing a diagnostic message, but I imagine we'll eventually have some test code that will rely on PyFITS, so we'd only be able to run the tests with it present.

Some of the meas_shape_hsm codes (meas_moments.c and meas_shape.c) would benefit from some minor improvements in user interface, though they are useful already. I can do this easily and quickly myself, but this issue is a reminder to do so once I'm back from my travels this week.

This is mainly a question for @TallJimbo, and not a hugely urgent one, but I've assigned it to myself as an Issue because I would like to fix it before too long, and for bookkeeping.

The Python wrappers for RNG (#31) have a slight quirk with the keyword arguments that I couldn't quite get around using the bp::optional< >. The docstring for Gaussian Deviate explains it best:

Initialization
----------------
g = GaussianDeviate(u, mean=0., sigma=1.) 

Initializes g to be a GaussianDeviate instance, and repeated calls to g() will
return successive, psuedo-random Gaussian deviates with specified mean and sigma.

Parameters:

u       a UniformDeviate instance (seed set there).
mean    semi-optional mean for Gaussian distribution (default = 0.).
sigma   optional sigma for Gaussian distribution (default = 1.).

The mean parameter is semi-optional: an ArgumentError exception will be raised if
sigma alone is specified without an accompanying mean. However, reversing their
ordering is handled OK provided keyword args are named. (TODO: Fix this 'feature'
if possible!)

The same issue effects the BinomialDeviate. Of course, we could get around this by making the mean and sigma arguments obligatory, but that seems inelegant given the frequency with which we are likely to want unit variance Gaussians with zero mean.

Any thoughts?

Currently the meas_shapes_hsm code relies on one NR routine, gaussjinv. (There are several NR codes in there that are officially open-source, such as dvector, ivector, dmatrix, etc. - those don't have to be removed, though we may wish to later on.) We should make the code use TMV for matrix inversion instead of gaussjinv; the result is that the code will probably be faster, anyway.

Please note: the ONLY place that the code requires gaussjinv is for the shapelets PSF correction, which is only one of several possible methods. If there is no clear and simple way to do this using TMV, we should simply remove gaussjinv and live without a shapelets PSF correction (the implementation in this code was not, in any case, very well tested, since we never used it for science!). This would be preferable in my opinion to someone spending a lot of time on getting this to work with TMV. Opinions on just how hard this replacement of gaussjinv --> TMV would be are therefore valuable!

If we choose to replace gaussjinv with something from TMV, this is probably a job for Mike. If we choose to rip it out and not have PSF correction via shapelets from this code, then this is a job for Rachel.

We should have the same scoped path to various bits of code in Python, C++, and in the on-disk layout of our headers.

I propose the following (note that I don't care much about the actual names, just the consistency):

galsim/artifacts/....
galsim/distort/...
...
galsim/sbprofile/_sbprofile.so
galsim/meas/hsm/_hsm.so
...
include/galsim/sbprofile/<sbprofile headers>
include/galsim/meas/hsm/<hsm headers>
...
src/sbprofile/<sbprofile source>
src/meas/hsm/<hsm source>
pysrc/sbprofile/<sbprofile wrapper code>
pysrc/meas/hsm/<hsm wrapper code>

In addition, the SBProfile C++ code should be in namespace galsim::sbprofile, and the HSM code should be in namespace galsim::meas::hsm. Of course, right now, HSM doesn't have any C++ headers, as it's just a C executable, but that would naturally be added if we convert it to a library with a Python interface.

Currently Simpson.h has a NR-based routine for integration by Simpson's rule, which Gary modified so it's written as a template with the integrand as the template argument. We should replace this with some alternative, such as Mike's integration package. Gary points out the decision to be made: whether to adopt the templating interface, or to change sbprofile to do more traditional function calls. The advantage of the template is that the integrand can be a class instead of a function, which allows better control of internal variables.

While doing the Image overhaul, it was a little scary to be modifying SBProfile code without any way to check that I hadn't broken anything.

It may be too much to work to write multi-level unit tests for the existing code in SBProfile, but I'd like to see some high-level regression tests (possibly making use of saved FITS outputs) to try to catch changes that break things. I don't think my Image changes will be the last ones that have the potential to do so.

We need unit tests for the Image class, as Jim's branch #12 does not include them yet.

I think the next step in the Python wrapping is to refactor the Image class so it can share data with NumPy arrays.

Here are the details of what I plan to do:

• Remove the FITSFile and FITSImage classes entirely.
• Remove the "header" component of Image.
• Reimplement Image internals to use boost::shared_ptr for reference counting; this can use an arbitrary deleter and will allow an Image to hold memory blocks allocated by NumPy.
• Re-write existing example code that uses FITS images in Python with PyFITS.
• Remove the CFITSIO bits from the build system.

I don't think it will be necessary to modify the Image public interface aside from removing the header access.

I plan to work on this on a branch...I'm tentatively planning to call it "issues/12". As this is the first feature branch, If you'd prefer descriptively-named branches, please speak up - I don't have a strong preference, but it's something we should decide on.

On at least two systems (Mandeep's and one of Mike's), there are errors compiling pysrc/Random.cpp.

g++ -o pysrc/.obj/Random.os -c -O2 -fno-strict-aliasing -g3 -Wall -Werror -fPIC -Iinclude/galsim -Iinclude -I/home/mjarvis/include -I/usr/local/cfitsio/include -I/usr/local/fftw/include -I/usr/include/python2.6 -I/usr/local/numpy/lib64/python2.6/site-packages/numpy/core/include pysrc/Random.cpp
In file included from include/galsim/Random.h:23,
                 from pysrc/Random.cpp:2:
include/galsim/boost1_48_0.random/mersenne_twister.hpp:493: error: a function call cannot appear in a constant-expression
include/galsim/boost1_48_0.random/mersenne_twister.hpp:493: error: a function call cannot appear in a constant-expression
include/galsim/boost1_48_0.random/mersenne_twister.hpp:494: error: a function call cannot appear in a constant-expression
include/galsim/boost1_48_0.random/mersenne_twister.hpp:494: error: a function call cannot appear in a constant-expression
include/galsim/boost1_48_0.random/mersenne_twister.hpp:495: error: a function call cannot appear in a constant-expression
include/galsim/boost1_48_0.random/mersenne_twister.hpp:495: error: template argument 6 is invalid
include/galsim/boost1_48_0.random/mersenne_twister.hpp:495: error: template argument 8 is invalid
include/galsim/boost1_48_0.random/mersenne_twister.hpp:495: error: template argument 10 is invalid
include/galsim/boost1_48_0.random/mersenne_twister.hpp:495: error: template argument 12 is invalid
include/galsim/boost1_48_0.random/mersenne_twister.hpp:495: error: template argument 14 is invalid
include/galsim/boost1_48_0.random/mersenne_twister.hpp:495: error: invalid type in declaration before ‘;’ token
scons: *** [pysrc/.obj/Random.os] Error 1

I think it is probably due to a mismatch of the version 1.48 boost files with whatever the other installed boost is, since the lines in question involve the UINT64_C macro, which is defined in boost/cstdint.hpp, and this is one of the files that Gary didn't port over into our boost1_48_0.random mirror. So I'm going to start by trying to copy over this and all other low level boost files that Gary left to the installed boost, so our mirror will be completely self-contained. Hopefully this will solve the problem...

This is something Barney mentioned. I'm not sure if it's needed for the first milestone, though.

SBProfile code Poisson.cpp uses some NR routines (gammp, gammq, gser, gcf) with very little alteration to build a Poisson deviate from a uniform deviate. Mike gave some references for implementing the incomplete gamma function:

http://en.wikipedia.org/wiki/Incomplete_gamma_function
http://oai.cwi.nl/oai/asset/10080/10080A.pdf
http://www.alglib.net/specialfunctions/incompletegamma.php
http://www.gnu.org/software/gsl/manual/html_node/Incomplete-Gamma-Functions.html

He points out that "The first two could be used to roll our own. The next two are GPL, so we could include those in our stack."

Per our discussion on the list, we want to make sure that the RNG we use (a) is open-source and (b) can be relied on to always produce the same random sequence given the same initial seed. Gary proposed and is implementing and documenting a solution that should fit those two requirements, so this issue is assigned to him (and is connected to #1 and #3, but I'm listing it separately as it is also a solution to other long-term problems).

Barney volunteered to make unit tests, which will be a separate issue.

Assigned: Barney, Rachel, Mike, Gary

We need a very simple module for adding simple Gaussian / Poisson(?) noise to our postage stamp galaxy images. This will be just the start: soon we will want the capability to add noise with a generic correlation function, including that which can compensate for correlated noise in the input image. However, to start with we will simply code up a placeholder function for adding basic noise.