regression tests for SBProfile about galsim HOT 39 CLOSED

Hmmm good point & idea. We can make a small set of reference images from a known-to-be "good" version of SBProfile, and run a comparison against these anytime we feel concerned that a change might have unforeseen consequences.

Were you imagining doing these tests in Python? If we made tests against reference versions of all the main SB<Profile_name> classes, and convolved / distorted combinations, it would also produce a nice demonstration of the wrapped interface as a bi-product. I'd be interested in writing such tests.

On Mar 19, 2012, at 1:29 PM, Jim Bosch wrote:

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.

---

Reply to this email directly or view it on GitHub:
#29

Barnaby Rowe
Postdoctoral Research Associate

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

from galsim.

After the Image (#12) merge, I think we'd have what we need to try to write such a thing in Python. And it would also be a good test to see if I have indeed wrapped all of the parts of SBProfile that would be useful to have access to in Python (right now, the RNG is the only part I know we need in Python that I haven't done).

from galsim.

I think we can combine this issue with #36...

from galsim.

Just wanted to mention that I ran the python version of the examples in https://github.com/GalSim-developers/GalSim/wiki/Examples:-getting-started-with-SBprofile-and-hsm , and the results were precisely the same as before. That tells us something, at least...

from galsim.

These begin to seem more important given that issue #49 has told us that some of the supposedly simple changes in SBProfile to conform to our coding standards introduced a platform-dependent bug. Barney, if you are very engrossed in some of the tasks you've taken on for the upcoming milestone, would you like me to take on at least a simple version of these tests this week? It would have an added benefit for me as a learning experience with nosetests, etc.

from galsim.

Hi Rachel,

Yes, if you wanted to do that, I think it would be great! It will be useful info regarding the use of the SBProfile classes too I don't think I'll have time to get to it this week, and you're right: we're seeing that it might be urgent. However, I think a full set of tests is a big job: I'd be tempted to prioritise certain things (i.e. not test all classes for now), we can always come back and do more. Thank you!

Barney

On 27 Mar 2012, at 20:30, Rachel Mandelbaum wrote:

These begin to seem more important given that issue #49 has told us that some of the supposedly simple changes in SBProfile to conform to our coding standards introduced a platform-dependent bug. Barney, if you are very engrossed in some of the tasks you've taken on for the upcoming milestone, would you like me to take on at least a simple version of these tests this week? It would have an added benefit for me as a learning experience with nosetests, etc.

---

Reply to this email directly or view it on GitHub:
#29 (comment)

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

from galsim.

Whoops responded to this last one as if it were a normal email! Will have to watch out for that... :)

from galsim.

I quite agree about it being a big job. I would propose to focus on tests of the basic SBProfile functionality that we'll be using for this milestone (i.e. creation of various profiles and doing simple operations of them -- but no SBPixel tests, for example). Once we have these basic tests in place we can have a bunch of people on different systems try them out to make sure that nothing else has been broken by the changes we've done to the code. Then we can do tests of other parts of SBProfile later.

Before I get going I want to run the plan by you (also pulling in @gbernstein as he might have some thoughts on this). There are two ways I can see doing these tests -

1. at the image level. This would require me to have a bunch of pre-computed images from the original C++ SBProfile code living in the repository, and write a python script to generate the same images using the current version of the software, then compare images directly. The things I worry about here are: (a) to have a really useful and extensive set of tests, that's a lot of data that has to be stuck in the repository, and (b) the comparison of images can be tricky. We don't expect or require the same answers to machine precision (for example, we've changed from Gary's NR-based integration to Mike's integ) just to some to-be-defined precision, and if we were to say we expect agreement in pixel values at the 10^{-5} level, this gets tricky in the outer parts of the image where the flux values themselves are very near zero.

2. at the level of image statistics. This would again require me to run the original C++ SBProfile and write a python script to make comparable images, but instead of comparing images I would simply have a long text file with statistics of the images from the original SBProfile such as image size; min, max, total flux; location of peak pixel within the image; and the second moments. I would then have to accumulate the same stats for the test images generated with the current version of GalSim. We would be assuming that we cannot have a bug that preserves all these things to reasonable precision but causes some other issue. Does that seem to be a safe assumption? I am inclined to think so... but perhaps I'm not sufficiently imaginative about how diabolical bugs can be.

So I am leaning towards (2), but would appreciate some feedback on this before I get started.

from galsim.

Sorry, more thoughts (again, pinging @gbernstein ):

For the base case, I'm planning to use the original SBProfile that Gary sent us with his Makefile and so on, using SBDraw to make images of various sorts. Probably I should check over the list of changes to the .cpp files to make sure there were no true bug fixes that I should incorporate (e.g., if the parser bugs that were identified in the beginning of tests for issue #49 were in the original code, then I would like to fix them). But I want to stay as close to the original as possible.

For testing, if I want to compare second moments then I originally thought to use hsm. But this seems like a bad idea since we don't currently have a python wrapper for it, so I'd have to run the hsm code from within the python test script by writing FITS images to file and using system calls. This is hideously clumsy. However, since the test images we're using have no added noise, perhaps we could bypass adaptive moments and use unweighted second moments.

from galsim.

Hi Rachel... Just a thought regarding your option 1) above. You can do this relatively easily (i.e. compare arrays at not quite machine precision) using some of the numpy.testing routines, e.g.

numpy.testing.assert_arrays_almost_equal(array1, array2, decimal)

If you want to compare to the 5th decimal place, you set decimal = 5. I used those routines for the DFT tests in the optics unit tests, and found them very easy to use for comparing arrays where we expect close agreement but not 100% congruence.

This would not answer the problem of having lots of FITS images in the repository. However, I'm not sure that these need be so huge... I would think that one 64 x 64 image would be enough for each case being tested, and for 8byte floats that's 32k per image + header & FITS gunk. If we had 30 such images around for tests, we hit a megabyte, which I agree is not ideal but liveable.

I think it's totally up to you, but I like the immediacy of an image when debugging: very often just loading it up will tell you exactly what's been broken (unless it's all zeros, an annoyingly common case as we've seen, or NaNs!). As you will need to make the reference images anyway to measure their moments, I don't think its any more work to just compare the images and swallow the Mb.

from galsim.

Thanks for pointing out that function to compare images, looks very handy! But what about cases, like what has come up with branch #49, where changes result in images having different sizes? That would require additional handling that could get cumbersome, since we'd have to identify that this is the case, and make sure we're taking appropriate sub-images for the comparison. In contrast if we compare statistics of the pixel values, second moments, etc. then those don't care how big the images are (except if we explicitly compare image size, but we might decide that it's fine for images to be different sizes as long as everything else about them is the same).

from galsim.

I would counter that the size change occurred because we fixed a bug. If the size changes again, would we not want tests that reflect that anyway? This is exactly the sort of thing that regression tests would want to flag up, I think...

from galsim.

(of course, you mention this in your parantheses... But I think we should definitely test for that, it's exactly the sort of clue we want to keep track of if there's something funny going on...)

from galsim.

Yes, we absolutely would. The second option I mentioned would flag it because we'd note the image size, but would also tell us that the images otherwise represent an object with the same characteristics as before. In the first option, we'd have to either (a) simply make the test fail because the images are not the same size, which is less informative, or (b) write some more elaborate code to find appropriate subimages to pass to the python test functions.

from galsim.

Just checked, np.testing.assert_array_almost_equal(a, b, decimal) does give an AssertionError if the arrays change shape, as well as if they change value, so this would potentially group at least these tests into one. But I'm very happy to have tests to stored moment estimates and stored, assumed correct output array sizes instead!

from galsim.

P.P.S. The AssertionError is informative, i.e. it tells you why the fail happened (following for a (4,5) and (5,5) array of zeros):

AssertionError: 
Arrays are not almost equal to 10 decimals

(shapes (5, 5), (4, 5) mismatch)
 x: array([[ 0.,  0.,  0.,  0.,  0.],
   [ 0.,  0.,  0.,  0.,  0.],
   [ 0.,  0.,  0.,  0.,  0.],...
 y: array([[ 0.,  0.,  0.,  0.,  0.],
   [ 0.,  0.,  0.,  0.,  0.],
   [ 0.,  0.,  0.,  0.,  0.],
   [ 0.,  0.,  0.,  0.,  0.]])

from galsim.

Okay, so unless someone else has an additional point to make, I guess I just have to decide whether I prefer having to stick lots of images in the repository, and handling size mismatches in an informative way, OR having to make tables containing various bits of information about the original images and write code that will do comparable tests for the new ones. I'm leaning towards the former, and we can re-evaluate if we find that to fully test SBProfile's capabilities we need huge numbers of test arrays.

from galsim.

It seems a good plan to me to have a few images produced checked for small residuals as part of the unit tests. The point is mostly to check for inadvertent changes to the output products (or just the ability to create them), and this would be the most straightforward way to do that. Taking moments might be useful if we need to compress the testing data volume, but right now we don't have to (my GalSim directory is 100 MB already, a few MB of small test images won't matter, and will be much smaller than any training set of galaxy images that we will use).

By the way, if you do want to measure moments, it can be quite simple for this purpose: no need to iterate centroids or anything, just take a moment around a pre-determined point and you'll have something that lets a unit test see changes.

On Mar 28, 2012, at 3:43 PM, Rachel Mandelbaum wrote:

Okay, so unless someone else has an additional point to make, I guess I just have to decide whether I prefer having to stick lots of images in the repository, and handling size mismatches in an informative way, OR having to make tables containing various bits of information about the original images and write code that will do comparable tests for the new ones. I'm leaning towards the former, and we can re-evaluate if we find that to fully test SBProfile's capabilities we need huge numbers of test arrays.

---

Reply to this email directly or view it on GitHub:
#29 (comment)

from galsim.

Update:
I just pushed a new branch in which I have done the initial work on this. Given that I've not done much programming in python before, I would appreciate it if someone could take a peek and see if (a) the way I'm doing the tests makes sense and (b) the tests I have chosen to do are reasonable. Once somebody has done a sanity check, I'll continue in a similar vein.

To give a little more detail: I have put in tests of the various SBProfiles, but not SBLaguerre or SBPixel. (That is, I generated test images for the various SBProfiles, but I only wrote a test for SBGaussian. The other test cases for other SBProfiles will be identical to this one in form.) The directory with test images is kind of large, mainly because of SBAiry. The second set of tests that I will include are of operations on SBProfiles, e.g., shearing, rotating, adding, convolving. I don't see any reason to add combinations of operations (e.g., shearing and convolving), but could be persuaded otherwise if someone has a good reason for it.

Currently, as you can see, the test uses numpy.testing.assert_array_almost_equal. I have commented-out code that tests for equality of the peak values, which would be necessary if I were going to compare image statistics (peak, min, total, size, second moments, etc.). I'm not going to develop that testing method further, but for now will rely on the numpy.testing functionality.

So, comments on the selection of things to test and/or of the first test that I wrote are welcome.

from galsim.

I think the tests comparing to reference images make sense. You can find more tests for SBGaussian and SBMoffat in branch #25_simple_atmosphere. I'm not sure the tests I wrote should be for atmosphere or tests for SBProfile, since right now I am only instantiating SBProfile classes.

One minor comment: Instead of explicitly writing slashes as path separators, you should us os.path.join to stay as platform independent as possible. Might be helpful, if we ever want to go to Windows. So,

imgdir = os.path.join(".", "SBProfile_comparison_images")

testImg = galsim.fits.read(os.path.join(imgdir, "gauss_1.fits"))

from galsim.

Joerg mentions a good point about the OS-independent folder name construction: I cannot say I am certain I've been 100% on this myself.

Rachel: I think what you have makes sense as a starting point, and is clear and easy to read too. Commented on a typo:
0cd99fd#commitcomment-1150980

otherwise think this looks great

from galsim.

Very good point, Joerg! I'll fix that, and the typo that Barney pointed out.

Thanks to both of you for the feedback, I will continue with this and send a pull request when ready.

from galsim.

Okay... I filled in the unit tests for the other 5 SBProfiles. The tests for Gaussian, Sersic, Airy, box, and Moffat passed; the test for exponential failed due to different array sizes. Despite its simplicity, I'm not completely in love with the numpy.testing.assert_array_almost_equal just because I would like to know more information, like does the object look the same just with more blank space around it, or is it actually a different-looking image. I'm completely new to numpy.testing; is there some way to get it to execute some additional bits of code if the test fails, rather than just telling me that it fails? (i.e., use it in an if statement, rather than just using it to announce the failure -- it seems like an obvious thing one might want to do... )

I also set up 9 tests of operations on SBProfiles. Here is where it gets a little dicey, because I'm relatively inexpert in python, so a failure could actually be a failure by me to define the test meaningfully. So far only 2 tests passed:

• convolution of two SBProfiles (in this case, Moffat and Box)
• flux rescaling (of a Sersic profile, though I have no reason to believe that success in this case depends on the choice of profile!)

5 of the tests failed due to mismatch in output array sizes compared to the saved outputs from the original SBProfile code:

• application of a small shear to a Gaussian
• application of a large shear to a Sersic
• 45 degree rotation of a sheared Sersic
• increase in size x 1.5 of an exponential (could conceivably have failed because of whatever problem caused the failure in the SBExponential unit test)
• addition of 2 flux-rescaled Gaussians

Clearly I need to investigate those further to figure out if it's a really substantive change in the results, or just a bunch of extra near-zero pixels around the edges.

2 of the tests failed in more interesting ways:

• the sheared Gaussian convolved with a box had 7% of the pixels disagreeing at >10^-5 level
• the translated Box function seems to suggest some confusion in x<-->y when applying a translation

I need to look into these more, as well, as they seem to be real problems (unless I set up the test incorrectly).

I didn't set up a pull request because I'm not totally convinced that I haven't abused / misused / misunderstood the python interface to SBProfile in some way that is causing failures, and because I want to figure out a way to be more informative on failure cases. If someone could take a quick look and see if these tests are sane, I would appreciate it. I don't believe we have to get to the bottom of all the failures as part of this issue (would be fine merging it in and opening new issues about these failures) but I do want to make sure that the tests are really doing what I want, and are sufficiently informative that we can figure out what's going on from the outputs.

from galsim.

For the if construct thing you mention, you have two options

i) Add another test for the thing you were thinking of if switching to anyway: all the tests get run and you get info on all the failures anyway.

ii) I think this will work use a try: except: construct: http://docs.python.org/tutorial/errors.html to handle the AssertionError exception that is getting thrown in the particular case of interest.

I'll be very happy to take a quick look at the other tests though, possibly later tonight or first thing tomorrow am!

On 29 Mar 2012, at 19:47, Rachel Mandelbaum wrote:

Okay... I filled in the unit tests for the other 5 SBProfiles. The tests for Gaussian, Sersic, Airy, box, and Moffat passed; the test for exponential failed due to different array sizes. Despite its simplicity, I'm not completely in love with the numpy.testing.assert_array_almost_equal just because I would like to know more information, like does the object look the same just with more blank space around it, or is it actually a different-looking image. I'm completely new to numpy.testing; is there some way to get it to execute some additional bits of code if the test fails, rather than just telling me that it fails? (i.e., use it in an if statement, rather than just using it to announce the failure -- it seems like an obvious thing one might want to do... )

I also set up 9 tests of operations on SBProfiles. Here is where it gets a little dicey, because I'm relatively inexpert in python, so a failure could actually be a failure by me to define the test meaningfully. So far only 2 tests passed:

• convolution of two SBProfiles (in this case, Moffat and Box)
• flux rescaling (of a Sersic profile, though I have no reason to believe that success in this case depends on the choice of profile!)

5 of the tests failed due to mismatch in output array sizes compared to the saved outputs from the original SBProfile code:

• application of a small shear to a Gaussian
• application of a large shear to a Sersic
• 45 degree rotation of a sheared Sersic
• increase in size x 1.5 of an exponential (could conceivably have failed because of whatever problem caused the failure in the SBExponential unit test)
• addition of 2 flux-rescaled Gaussians

Clearly I need to investigate those further to figure out if it's a really substantive change in the results, or just a bunch of extra near-zero pixels around the edges.

2 of the tests failed in more interesting ways:

• the sheared Gaussian convolved with a box had 7% of the pixels disagreeing at >10^-5 level
• the translated Box function seems to suggest some confusion in x<-->y when applying a translation

I need to look into these more, as well, as they seem to be real problems (unless I set up the test incorrectly).

I didn't set up a pull request because I'm not totally convinced that I haven't abused / misused / misunderstood the python interface to SBProfile in some way that is causing failures, and because I want to figure out a way to be more informative on failure cases. If someone could take a quick look and see if these tests are sane, I would appreciate it. I don't believe we have to get to the bottom of all the failures as part of this issue (would be fine merging it in and opening new issues about these failures) but I do want to make sure that the tests are really doing what I want, and are sufficiently informative that we can figure out what's going on from the outputs.

---

Reply to this email directly or view it on GitHub:
#29 (comment)

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

from galsim.

Yeah, I should probably just do (i). For the tests that pass, it doesn't hurt to run a few more redundant tests.

from galsim.

And since the first failed test using the numpy.assert* functions means that the rest are not run, the way I'm doing this is that before the test, I use print statements with the values of interest. Because of the way nosetests works, print statements are only reported if a test fails. So this is pretty much the behavior I'm looking for, and we won't have tons of diagnostic information printed out for the tests that passed.

from galsim.

Great!

On 30 Mar 2012, at 07:49, Rachel Mandelbaum wrote:

And since the first failed test using the numpy.assert* functions means that the rest are not run, the way I'm doing this is that before the test, I use print statements with the values of interest. Because of the way nosetests works, print statements are only reported if a test fails. So this is pretty much the behavior I'm looking for, and we won't have tons of diagnostic information printed out for the tests that passed.

---

Reply to this email directly or view it on GitHub:
#29 (comment)

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

from galsim.

I've just pushed a version of the code that includes more diagnostic information: image sizes, min/max pixel values, sum over pixels, peak location, and moments. I'm afraid that this code is not very elegant because I'm so new to python; I would very much appreciate any constructive criticism that any of you can offer! (could wait for the pull request, though, if people are too busy at the moment)

In any case, using this more informative code, I have concluded that:

(1) there might be a real issue in SBExponential. The Mxx and Myy are 2.8x as large as for the reference image, and I can't find an error in the test itself, so I will open a separate issue for this unless someone can think of a problem with this test. Because the magnification test also uses an exponential, that test fails (or, it might fail because there is something wrong with magnification too, but we won't know that until the SBExponential issue is fixed).

(2) there is a failure in the test of addition of two SBProfiles. Can someone look to see if I've misunderstood how to do this in python using SBAdd? Currently the code is like this:

mySBP = galsim.SBGaussian(1)
mySBP.setFlux(0.75)
mySBP2 = galsim.SBGaussian(3)
mySBP2.setFlux(0.25)
myAdd = galsim.SBAdd(mySBP,mySBP2)
myImg = myAdd.draw(dx=0.2)

to get an Image of a double Gaussian with total flux = 1.

Otherwise, I am pleased to say that the tests pass for all SBProfile classes except SBExponential (and did not try SBLaguerre or SBPixel), and the tests of shearing, convolution, translating, flux rescaling, and rotating all pass.

Once I understand whether the 2nd issue is because of a poorly-designed test or a real issue, and fix the problem if it turns out to be the former, I'll send a pull request and consider the first-pass unit tests to be complete. Will need to make these more thorough, and add tests for SBPixel later on...

from galsim.

Rachel - I missed something at the start of the thread: what are the reference images for the comparison, i.e. what does it mean for a test to fail?

One thing to check on the exponential is if all the constructors being compared have the same convention for defining radius (is it half-light, or e-folding length, FWHM, etc.).

On Mar 30, 2012, at 1:40 PM, Rachel Mandelbaum wrote:

I've just pushed a version of the code that includes more diagnostic information: image sizes, min/max pixel values, sum over pixels, peak location, and moments. I'm afraid that this code is not very elegant because I'm so new to python; I would very much appreciate any constructive criticism that any of you can offer! (could wait for the pull request, though, if people are too busy at the moment)

In any case, using this more informative code, I have concluded that:

(1) there might be a real issue in SBExponential. The Mxx and Myy are 2.8x as large as for the reference image, and I can't find an error in the test itself, so I will open a separate issue for this unless someone can think of a problem with this test. Because the magnification test also uses an exponential, that test fails (or, it might fail because there is something wrong with magnification too, but we won't know that until the SBExponential issue is fixed).

(2) there is a failure in the test of addition of two SBProfiles. Can someone look to see if I've misunderstood how to do this in python using SBAdd? Currently the code is like this:

mySBP = galsim.SBGaussian(1)
mySBP.setFlux(0.75)
mySBP2 = galsim.SBGaussian(3)
mySBP2.setFlux(0.25)
myAdd = galsim.SBAdd(mySBP,mySBP2)
myImg = myAdd.draw(dx=0.2)

to get an Image of a double Gaussian with total flux = 1.

Otherwise, I am pleased to say that the tests pass for all SBProfile classes except SBExponential (and did not try SBLaguerre or SBPixel), and the tests of shearing, convolution, translating, flux rescaling, and rotating all pass.

Once I understand whether the 2nd issue is because of a poorly-designed test or a real issue, and fix the problem if it turns out to be the former, I'll send a pull request and consider the first-pass unit tests to be complete. Will need to make these more thorough, and add tests for SBPixel later on...

---

Reply to this email directly or view it on GitHub:
#29 (comment)

from galsim.

The reference images come from the initial version of SBProfile that you sent me and Barney, before ANY changes. So the convention for defining radius should be the same.

from galsim.

To clarify, the point of this choice was to ensure that we haven't broken SBProfile whenever we've mucked around in it. We assume that external validation / verification will happen separately.

from galsim.

I'm not yet literate enough to run the python tests.
But the 2nd moment of the exponential is analytic, Mxx and Myy should each be equal to 3 r0^2 (when normalized by flux). Which is giving the right answer - old or new SBProfile?
And likewise the double-Gaussian - can you tell which (if either) is giving the right moments? (or maybe this is not the nature of the failure with this case).

On Apr 2, 2012, at 9:49 AM, Rachel Mandelbaum wrote:

To clarify, the point of this choice was to ensure that we haven't broken SBProfile whenever we've mucked around in it. We assume that external validation / verification will happen separately.

---

Reply to this email directly or view it on GitHub:
#29 (comment)

from galsim.

Gary, when I make the pull request I'll put an explanation of how to run the python tests.

Good point about checking the moments analytically. If I've properly understood what you meant about "normalized by flux" then for the exponential, it's actually the new one that is correct (almost: when I specify a radius of 1, the new one gives Mxx=Myy=2.9 and the old one gives 1.0). But actually, by r0 do you mean r_e? I think all inputs are r_e, not r0 in the I_0 propto exp(-(r/r0)^(1/n)), right?

from galsim.

Now I'm beginning to suspect a difference in definitions, because the difference between r_e and r0 for exponential n=1 is 1.68, and 1.68^2 is pretty close to the change in moments from new vs. old.

To be clear, what I've been doing is this:

• the saved image on disk comes from using your original SBProfile and putting in radius of 1. Now, by reading the old SBParse, I see that this radius is meant to be r_e, so r_0=1/1.68, and it's 1/1.68 that gets passed to the constructor for the SBExponential.
• the new image comes from using the python, which I believe just takes r_0.

I'm 99% sure that this is the explanation - and this is consistent with the fact that we get the right answer for all the other SBProfiles, because I believe that SBExponential is the only one that has a radius definition that gets explicitly changed by SBParse. I'll fix it.

from galsim.

Yes, that's right, SBParse treats an exponential differently than the constructor does.
Feel free to decide what is the best convention for your use; obviously I vacillated between thinking that users would want a scale length for exponential disks to wanting to have as many of the radii be half-light as possible.

On Apr 2, 2012, at 10:33 AM, Rachel Mandelbaum wrote:

Now I'm beginning to suspect a difference in definitions, because the difference between r_e and r0 for exponential n=1 is 1.68, and 1.68^2 is pretty close to the change in moments from new vs. old.

To be clear, what I've been doing is this:

• the saved image on disk comes from using your original SBProfile and putting in radius of 1. Now, by reading the old SBParse, I see that this radius is meant to be r_e, so r_0=1/1.68, and it's 1/1.68 that gets passed to the constructor for the SBExponential.
• the new image comes from using the python, which I believe just takes r_0.

I'm 99% sure that this is the explanation - and this is consistent with the fact that we get the right answer for all the other SBProfiles, because I believe that SBExponential is the only one that has a radius definition that gets explicitly changed by SBParse. I'll fix it.

---

Reply to this email directly or view it on GitHub:
#29 (comment)

from galsim.

I think we can decide later about the best convention, but for now I just want to understand what is the difference between these images. Given that the saved image had re=1 due to the SBParse convention, I changed the test so that it used r0=1/1.68. And in doing so, I realized another silly thing I had been doing, which is that some of the optional parameters to SBDraw cannot be skipped in the python-wrapped SBProfile without making this clear explicitly - i.e., I had been leaving out parameter names and it was assuming a certain order for parameters, and thus the code was misinterpreting the arguments I was giving it. I've fixed that, and now ALL tests pass.

Will send a pull request shortly, and open an issue for some of these differences in notation (r0 vs re) in case we want a different convention.

from galsim.

I just merged these now; of course, comments on what I've done are still useful. I'm leaving the issue open because in the not-too-distant future we need to supplement with tests of SBPixel etc.

from galsim.

Sorry I took so long in taking a look at this. It all looks pretty good. My only comment is that you're doing a lot of printing comparisons that it'd also be nice to do explicit test checks on, and I suspect that's because the only actual testing tool you're using is the one from numpy.testing. I get the impression that nosetests will treat any Python exception as a test failure, and it's common practice to just use the Python assert statement for that (see test_Image.py for an example) but nose also provides appears to provide some convenience functions to make a lot of those easier:

http://readthedocs.org/docs/nose/en/latest/testing_tools.html#tools-for-testing

...though I suspect there must be much better documentation on this functionality somewhere. Maybe Joe can recommend something?

Anyhow, with those, you could very easily rewrite the printvals function with something that checked each of the printed quantities was actually equal, and only printed them when they weren't (of course, it may be desirable to print them all the time - and in that case you may want to write a custom test function that always prints two values and asserts that they're equal).

from galsim.

Thanks for this, Jim!

The rationale for my current design is this:

• if the test of image equality passes, then that tells us everything we need to know. And when a test passes, nosetests does not print anything from stdout (it just throws it away), so those printed comparisons don't clutter up the output. They do take a little time, but not much...
• if the test of the image equality fails, then we do want to see ALL those printed comparisons, because they might help us diagnose what's wrong. If the test of image equality fails, then nosetests will tell the user about everything that went to stdout from printvals(). (Note, we can't do this using another numpy.testing tool, because after the first failure it doesn't run anything else in that test. So if the image test fails, we are done. If we try to print out this information before the image test, but use a numpy or nose tests, then in that case it also only goes until the first failure.)

So i agree that this is clumsy, but it seemed like the best way to go given that within a given test, nosetests stops after the first failure (and that is also true using nose tools like nose.tools.eq_). Or have I misunderstood something about nose?

I'm open to advice on specific usage that might fix the problem, but I didn't see examples that addressed this particular issue of wanting more information only in case of failure.

from galsim.