DM-9882 Add integrate interface to BoundedField/ChebyshevBoundedField #197
Conversation
parejkoj
force-pushed
the
tickets/DM-9882
branch
from
07fadcf
to
c061194
Compare
tests/testChebyshevBoundedField.py
Outdated
| The values of these integrals were checked in Mathematica. The code | ||
| block below can be pasted into Mathematica to re-do those calculations. | ||
|
|
||
| .. code:: mathematica |
There was a problem hiding this comment.
This may not work as I don't see mathematica listed on Pygment's supported languages. You could just do a general pre-formatted block:
.. code:: mathematica
to
::
On the other hand, we're not planning rendering unit test docstrings in Sphinx, so this isn't a significant point.
include/lsst/afw/math/BoundedField.h
Outdated
| @@ -37,6 +37,9 @@ namespace lsst { namespace afw { namespace math { | |||
| /** | |||
| * @brief An abstract base class for 2-d functions defined on an integer bounding boxes | |||
| * | |||
| * Integer bounding boxes (afw.geom.Box2I) are inclusive of the end pixels, thus a BoundedField | |||
| * is defined on the inclusive box [x0, x1] x [y0, y1]. | |||
There was a problem hiding this comment.
I think you need something stronger than this if you really want to get at what's not intuitive about these boxes. Maybe something like this "because integer positions correspond to the centers of pixels, and these boxes include the entirety of those pixels, the actual range covered by the BoundedField is [x0 - 0.5, x1 + 0.5] x [y0 - 0.5, y1 + 0.5]."
include/lsst/afw/math/BoundedField.h
Outdated
| * @brief Compute the integral of this function over its bounding-box. | ||
| * | ||
| * @return The value of the integral. | ||
| */ |
There was a problem hiding this comment.
I don't think we normally do all these extra spaces between the Doxygen tag and the text they correspond to. I also don't think the @brief tags are actually necessary; Doxygen will automatically treat the first line as the brief summary.
There was a problem hiding this comment.
I need to reconfigure my DoxyDoxygen settings probably. Fixed.
src/math/BoundedField.cc
Outdated
| @@ -38,6 +38,16 @@ ndarray::Array<double,1,1> BoundedField::evaluate( | |||
| return out; | |||
| } | |||
|
|
|||
| double BoundedField::integrate() const | |||
| { | |||
There was a problem hiding this comment.
Open parens should go on the same line as the function declaration. Same comment in several places below.
tests/testChebyshevBoundedField.py
Outdated
|
|
||
| # 2nd order polynomial in x, 0th in y | ||
| coeffs = np.array([[5., 0., 7.]]) | ||
| self._testIntegrateBox(bbox, coeffs, 4.*coeffs[0, 0] - 4./3.*coeffs[0, 2]) |
There was a problem hiding this comment.
Please add some parens around the 4./3. here for clarity. Same below.
src/math/ChebyshevBoundedField.cc
Outdated
|
|
||
| double ChebyshevBoundedField::integrate() const { | ||
| double result = 0; | ||
| double determinant = getBBox().getArea() / 4.; |
There was a problem hiding this comment.
Coding standards for C++ say we should use 4.0, not 4. (I think we're free to do either in Python, formally, but I think it'd be best to use the same formatting there, too).
parejkoj
force-pushed
the
tickets/DM-9882
branch
2 times, most recently
from
0ce035d
to
0ed5f95
Compare
Add description of polynomial form of Chebyshev and example Add clarifying note about inclusivity of BoundedField
integrate() and mean() are virtual functions in BoundedField, with the only current implementation in ChebyshevBF. Tests were written wtih the help of Mathematica (sample code block included in test docstring).
parejkoj
force-pushed
the
tickets/DM-9882
branch
from
0ed5f95
to
dbf67db
Compare
No description provided.