DOxygen-style documentation for sbprofile C++ code about galsim HOT 18 CLOSED

Yep, this is my job for this week!

from galsim.

Hi Gary, I've made a first attempt at documenting all the classes and subclasses within SBProfile.h. This doesn't therefore include all the SBProfile stuff, such as SBDeconvolve or SBPixel, but it seemed like a natural place to stop and ask for a sanity check from you!

To generate the documentation (it should be fairly straightforward, although I managed to puzzle myself while getting going generating the docs!) see Joe's helpful docs/doxygen_readme.txt in the repo.

If you then open html/index.html and follow the links Packages > galsim you should, after scrolling down past the alphabetically-listed non-commented classes, see the following links accompanied with these short descriptions:

class SBError
Outputs SBProfile error message to stderr. More...

class SBProfile
An abstract base class representing all of the 2D surface brightness profiles that we know how to draw. More...

class SBAdd
Sums SBProfiles. More...

class SBDistort
An affine transformation of another SBProfile. More...

class SBConvolve
Convolve SBProfiles. More...

class SBGaussian
Gaussian Surface Brightness Profile. More...

class SBSersic
Sersic Surface Brightness Profile. More...

class SBExponential
Exponential Surface Brightness Profile. More...

class SBAiry
Surface Brightness Profile for the Airy disk (perfect diffraction-limited PSF for a circular aperture), with central obscuration. More...

class SBBox
Surface Brightness Profile for the Boxcar function. More...

class SBLaguerre
Class for describing Gauss-Laguerre polynomial Surface Brightness Profiles. More...

class SBMoffat
Surface Brightness for the Moffat Profile (an approximate description of ground-based PSFs). More...

class SBRotate
This class is for backwards compatibility; prefer rotate() method. More...

class SBDeVaucouleurs
Surface Brightness for the de Vaucouleurs Profile, a special case of the Sersic with n = 4. More...

These are the classes I've documented. I would love it if you had a chance to take a look and correct any errors: your own documentation was invaluable, there were a few times I went into the C++ to get more detail. I am, however, somewhat new to C++ and so might have either made conceptual errors there, or possibly elsewhere. Hopefully not, but I couldn't comment on the likelihood except that I made efforts to try and avoid this!

If there are problems/corrections, let me know. Many of the doxygen-ized methods that are shared by all the SBProfile subclasses are all documented in one place (i.e. in SBProfile), so making global corrections is straightforward.

P.S. One difference I've noted between Doxygen 1.7.4 and 1.8.0 is that in the latter, apart from seeming to be slightly more stable, will mark up code nicely marked look like code in the doc text, as it is here in Github. In 1.7.4 this syntax simply prints "'code'", which is bearable but a little annoying!

from galsim.

Hi Gary, I've also added SBPixel.h now (although not Interpolant.h), and corrected a couple of minor things I spotted in SBProfile.h dox. When you take a look, just run DOxygen using whatever is the most recent entry in the master branch.

from galsim.

Barney - here are comments going through the DOx that you wrote for the SBProfile classes. I am guessing you'd rather read explanations/clarifications here than just have me change the documentation strings, but maybe I'm wrong…

SBError is an exception class (derives from std::runtime_error). The argument of its constructor just becomes the error message that is accessible using std::runtime_error::what() method, which is commonly sent to cerr after the exception is caught. But the doc is wrong in saying that the class writes the string to stderr.

SBPixel: if you want to list all the derived classes in the detailed description, you need to add SBPixel and SBDeconvolve (which however are defined in different .h files).

SBPixel::centroid(): a design issue, not doc issue: you probably want to make this virtual, since a derived class may have economies of calculating x & y together instead of serially as the current definition requires (example: SBDistort)

SBPIxel::draw(): may wish to note that if you give an input image, its origin may be redefined by the time it comes back.

SBPixel::drawK(): need to perhaps get rid of statement that "The default draw() routines decide internally whether image can be drawn directly in real space or needs to be done via FFT from k space." which was copied from draw() method but isn't relevant here.

SBProfile::getFlux(): returns the total flux of the object - not sure if that's what you mean by "flux scaling."
SBProfile::setFlux(): same comment.

SBProfile::rotate(), shear(), shift(): might wish to note that this returns ptr to new SBProfile that represents a new SBProfile object with the requested transformation. The type of the new object is currently SBDistort, but that is an implementation choice, should not be assumed.

SBProfile::xValue(): note that "not implemented" means it will throw SBError, since C++ won't let you construct a class that does not have a concrete implementation of every method.

SBAdd::isAnalyticK() and many others: looks like DOxygen automatically copies the base class docs. But is it desirable to describe derived class's behavior, e.g. in this case the SBAdd is considered "analytic" if and only if all of its summands are? This is really a "physics" documentation, not coding documentation. Likewise such things as maxK().

SBDistort: I don't think it's required that det(M)=1. (De-)magnifications are allowed. But det(M)=0 will cause problems.

I haven't delved into the Laguerre docs - that's a bit off our beaten path, not your highest priority probably.

Otherwise everything looks crystal clear to me :)

g

On Mar 19, 2012, at 8:56 PM, Barnaby Rowe wrote:

Hi Gary, I've also added SBPixel.h now (although not Interpolant.h), and corrected a couple of minor things I spotted in SBProfile.h dox. When you take a look, just run DOxygen using whatever is the most recent entry in the master branch.

---

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

from galsim.

Great, thank you, I'll make edits based on your suggestions tomorrow morning and might get back in touch if I have further questions...!

Sent from my iPhone

On Mar 20, 2012, at 7:19 PM, Gary [email protected] wrote:

Barney - here are comments going through the DOx that you wrote for the SBProfile classes. I am guessing you'd rather read explanations/clarifications here than just have me change the documentation strings, but maybe I'm wrong…

SBError is an exception class (derives from std::runtime_error). The argument of its constructor just becomes the error message that is accessible using std::runtime_error::what() method, which is commonly sent to cerr after the exception is caught. But the doc is wrong in saying that the class writes the string to stderr.

SBPixel: if you want to list all the derived classes in the detailed description, you need to add SBPixel and SBDeconvolve (which however are defined in different .h files).

SBPixel::centroid(): a design issue, not doc issue: you probably want to make this virtual, since a derived class may have economies of calculating x & y together instead of serially as the current definition requires (example: SBDistort)

SBPIxel::draw(): may wish to note that if you give an input image, its origin may be redefined by the time it comes back.

SBPixel::drawK(): need to perhaps get rid of statement that "The default draw() routines decide internally whether image can be drawn directly in real space or needs to be done via FFT from k space." which was copied from draw() method but isn't relevant here.

SBProfile::getFlux(): returns the total flux of the object - not sure if that's what you mean by "flux scaling."
SBProfile::setFlux(): same comment.

SBProfile::rotate(), shear(), shift(): might wish to note that this returns ptr to new SBProfile that represents a new SBProfile object with the requested transformation. The type of the new object is currently SBDistort, but that is an implementation choice, should not be assumed.

SBProfile::xValue(): note that "not implemented" means it will throw SBError, since C++ won't let you construct a class that does not have a concrete implementation of every method.

SBAdd::isAnalyticK() and many others: looks like DOxygen automatically copies the base class docs. But is it desirable to describe derived class's behavior, e.g. in this case the SBAdd is considered "analytic" if and only if all of its summands are? This is really a "physics" documentation, not coding documentation. Likewise such things as maxK().

SBDistort: I don't think it's required that det(M)=1. (De-)magnifications are allowed. But det(M)=0 will cause problems.

I haven't delved into the Laguerre docs - that's a bit off our beaten path, not your highest priority probably.

Otherwise everything looks crystal clear to me :)

g

On Mar 19, 2012, at 8:56 PM, Barnaby Rowe wrote:

Hi Gary, I've also added SBPixel.h now (although not Interpolant.h), and corrected a couple of minor things I spotted in SBProfile.h dox. When you take a look, just run DOxygen using whatever is the most recent entry in the master branch.

---

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

---

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

from galsim.

Hi Gary,

Sorry that took longer than I expected, in the mean time I had to redo all the Dox in the file to make them match a revised set of conventions.

Some revisions, based on the easy to address points you raised above, are now in the master. But I have a couple of questions below...

Sorry too that (as you may have guessed), I am learning the finer points of C++ along the way with this project. All good for my brain, and I am enjoying it, but that is the reason for some of the dumber errors!

My questions:

i) The SBPixel::centroid() is actually defined at the SBProfile level. Here's what was in the original commit for SBProfile.h:

Position<double> centroid() const {
  Position<double> p(centroidX(),centroidY());  return p; }

There is no actual declaration in SBPixel.h, and checking back through the repo record I don't think I accidentally deleted one. Would you like me to make that virtual at the SBProfile.h declaration, within the SBProfile base class? Other member methods, such as centroidX() and centroidY() are made virtual there, but I just assumed that there was something specific (and unknown to me) about Position types that meant they couldn't be virtual. Will do whatever is best!

ii) In SBPixel.h the SBPixel::draw() method isn't declared, again this comes from SBProfile. But I think your point about adding a warning that the centroid might change is a good idea. I'm assuming it's OK for me to redeclare the various overloadings of the draw() method in SBPixel.h so that this warning can be added?

iii) As regards to documenting the isAnalytic() - type methods at the derived class level: yes, DOxygen currently does just copy from the base class. I've added a TODO there on those statements, because I think you're right it would be good to have a more derived-class specific doc statement, but unfortunately I'mm currently way behind on learning how to wrap a C++ RNG into Python... Is that OK with you for now?

iv) The detM=1 thing comes from the following lines (~242-245 in the original SBProfile.h that was sent out):

class SBDistort: public SBProfile {
// keep track of all distortions in a 2x2 matrix M = (A B) (detM=1)
// x0 is shift                                       (C D)
// 

I know that you often adopt a unit-determinant ellipticity definition, and so I thought that perhaps this incorporated into your definition of the M matrix, with the ability to magnify handled by some additional multiplier, maybe absdet.

Would you be able to clarify that a little bit? I've removed many of the detM=1 references in the dox (a million as I had before is overkill) but I'd like to understand this question myself anyhow, so we can design a python distortion class that follows the same prescription!

Thank you!

from galsim.

Hi Barney - responses inserted below…

On Mar 22, 2012, at 3:33 PM, Barnaby Rowe wrote:

Hi Gary,

Sorry that took longer than I expected, in the mean time I had to redo all the Dox in the file to make them match a revised set of conventions.

Some revisions, based on the easy to address points you raised above, are now in the master. But I have a couple of questions below...

Sorry too that (as you may have guessed), I am learning the finer points of C++ along the way with this project. All good for my brain, and I am enjoying it, but that is the reason for some of the dumber errors!

My questions:

i) The SBPixel::centroid() is actually defined at the SBProfile level. Here's what was in the original commit for SBProfile.h:

Position centroid() const {
Position p(centroidX(),centroidY()); return p; }

All I'd suggest is to make this a virtual function by adding the word "virtual" before those lines in the SBProfile class definition. This way if a derived class has a more efficient way than running centroidX and centroidY serially, it can override the base class default and use the more efficient method. Which is basically what you say below…

There is no actual declaration in SBPixel.h, and checking back through the repo record I don't think I accidentally deleted one. Would you like me to make that virtual at the SBProfile.h declaration, within the SBProfile base class? Other member methods, such as centroidX() and centroidY() are made virtual there, but I just assumed that there was something specific (and unknown to me) about Position types that meant they couldn't be virtual. Will do whatever is best!

ii) In SBPixel.h the SBPixel::draw() method isn't declared, again this comes from SBProfile. But I think your point about adding a warning that the centroid might change is a good idea. I'm assuming it's OK for me to redeclare the various overloadings of the draw() method in SBPixel.h so that this warning can be added?

I think this warning applies to all of the draw() methods with an Image argument, so putting the warning in the base class SBProfile::draw() docs would be fine. Maybe I mistakenly wrote SBPixel::draw() in my message to you when I meant to say SBProfile::draw()?

iii) As regards to documenting the isAnalytic() - type methods at the derived class level: yes, DOxygen currently does just copy from the base class. I've added a TODO there on those statements, because I think you're right it would be good to have a more derived-class specific doc statement, but unfortunately I'mm currently way behind on learning how to wrap a C++ RNG into Python... Is that OK with you for now?

Not a problem!

iv) The detM=1 thing comes from the following lines (~242-245 in the original SBProfile.h that was sent out):

class SBDistort: public SBProfile {
// keep track of all distortions in a 2x2 matrix M = (A B) (detM=1)
// x0 is shift (C D)
//

I know that you often adopt a unit-determinant ellipticity definition, and so I thought that perhaps this incorporated into your definition of the M matrix, with the ability to magnify handled by some additional multiplier, maybe absdet.

I'm not sure what that comment in my code was ever meant to mean. We should take it out. The SBDistort class allows any values for a,b,c,d. The "absdet" and "invdet" members are calculated and cached for two reasons:
(1) the inverse mapping requires invdet, so it's cached instead of recalculated every time
(2) the distortion is defined to conserve surface brightness in the x domain. Therefore a factor of absdet has to be applied in the k domain, which shows up in the kValue() calls.

The consequence is that a distort with |M| != 1 will change the flux (and in fact this is how SBDistort::flux() works too).

I might cause some of this confusion by defining shear matrices to have unit determinant. So if you look at the code for the Ellipse class (in Shear.h) you will see that, by default, when you ask for an ellipse with shear (e1,e2) you will get a unit-determinant matrix being used. (As opposed e.g. to common use of M = diag(1+\gamma, 1-\gamma) which has a flux magnification of 1-\gamma^2). When you build an Ellipse you can specify a magnification separately (in fact you have to give ln(magnification), another one of my affectations), so the Ellipse class does allow for any non-rotational affine transformation to be described. SBDistort is more general since you can have rotations if you want. I am not sure that the Ellipse class will be considered useful for GalSim, perhaps deleting it will lead to less confusion.

g

from galsim.

I have not followed the above very closely, but regarding the last point, I would add this:

We are definitely planning to define our own distortion class, which presumably would replace the ellipse class. However, there is still a value to documenting the ellipse class carefully because we need to make sure that we're doing something sane when we do that replacement. Also will be useful when setting up the python user interface to make sure we know exactly what conventions are used where.

Ideally, I would eventually like it if the python interface to let the user explicitly choose between a range of common parameterizations for distortions, with us having routines to quietly get everything into a common parameterization that we use for all internal computations.

But that's getting kind of far afield from this issue!

from galsim.

OK, thanks Gary! As adding the "virtual" potentially changes code I ought to put it it in a separate branch and pull request for consistency... I'll reference this issue there. As for the other clarifications, thank you, (I think indeed you originally meant SBProfile::draw(), which actually makes the change much more trivial) I'll work on that in the branch and then we can take a good look before merging the pull request....

from galsim.

On Mar 23, 2012, at 12:09 PM, Rachel Mandelbaum wrote:

Ideally, I would eventually like it if the python interface to let the user explicitly choose between a range of common parameterizations for distortions, with us having routines to quietly get everything into a common parameterization that we use for all internal computations.

Rachel, the Shear.h class in SBProfile was written with this in mind, namely that you could input or output shears with different conventions (a^2-b^2 / a^2 + b^2 vs a-b/a+b, etc) but the object does conversions as needed so that all transformations are consistent. In other words you can use several different definitions of ellipticity to describe the same coordinate transformation. The Ellipse class uses the Shear class, they're both in Shear.h.

But Shear.h always assumes you want a unit-determinant transformation matrix. Once there are multiple possible coordinate transformations associated with a single (e1,e2) pair, then the user must make some explicit choices and there is no way to have this be invisible.

from galsim.

Ah ha, and now I see what happens when I start learning SBProfile based on the .h files in alphabetical order... I'm still wading through SBProfile.h, haven't gotten to Shear.h. :)

As for your second point, we should keep this in mind... I think we have some decisions to make overall about conventions for coord transforms (and I'd like to make choices that will lead to a natural / simple interface once we include magnification, too).

from galsim.

Hi all, in light of the discussion in #105, Rachel made the excellent suggestion to move some of the contents of docs/SBProfile.pdf into the actual documentation of the code, and DOxify it. This would involve fleshing out some of the descriptions, but neither or Rachel or myself can think of a reason not to do this (within reason, of course). I think it fits in with this Issue, so I've mentioned it here and will start doing it soon (probably soon after the next Milestone).

from galsim.

It looks like most of the work on this relates to the Shear class. Suggest merging this into #134, updating it for any changes required for what Rachel did there, and then including it with the current pull request for that.

(I think the rest of SBProfile is pretty well doxygenated at this point.)

from galsim.

Sounds like a plan, had been keeping this ticking over without much progress for a while! I'll merge this into #134 (and fix any discrepancies) if @rmandelb has no objections?

from galsim.

No objections. Let me know if I can help with resolution of any discrepancies.

On Jun 18, 2012, at 2:48 PM, Barnaby Rowe wrote:

Sounds like a plan, had been keeping this ticking over without much progress for a while! I'll merge this into #134 (and fix any discrepancies) if @rmandelb has no objections?

---

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

Rachel Mandelbaum
http://www.astro.princeton.edu/~rmandelb
[email protected]

from galsim.

So shall we close this now that its been merged?

from galsim.

Yes.

from galsim.

Thanks for merging!

On 18 Jun 2012, at 18:36, Mike Jarvis wrote:

Yes.

---

Reply to this email directly or view it on GitHub:
#21 (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.