DM-16305: Implement bbox integrator and proper photoCalib mean #119
Conversation
parejkoj
force-pushed
the
tickets/DM-16305
branch
5 times, most recently
from
69709f9
to
c023ec4
Compare
parejkoj
force-pushed
the
tickets/DM-16305
branch
from
c023ec4
to
a98b44e
Compare
|
Ugh. Github is showing these commits out of order, and even |
| // Compute the integral of this function over its bounding-box. | ||
| double integrate() const; | ||
| // One portion of a definite 2-d integral. | ||
| double oneIntegral(double x, double y) const; |
There was a problem hiding this comment.
Future you will want a mathematical definition of "one portion", I think.
| // recurrence relation: integral(Tn(x)) = n*T[n+1](x)/(n^2 - 1) - x*T[n](x)/(n-1) | ||
| integralTmy[i] = i * Tmy[i + 1] / (i * i - 1) - point.getY() * Tmy[i] / (i - 1); | ||
| integralTnx[i] = i * Tnx[i + 1] / (i * i - 1) - point.getX() * Tnx[i] / (i - 1); | ||
| } |
There was a problem hiding this comment.
Everything above is duplicated for x and y. Can you separate that out into a 1-d function that's called twice?
Incidentally, this is also the part that I think would fit will in lsst::geom::polynomials; like the polynomials themselves, the integrals can be evaluated via:
- a value for the zeroth term (i.e. line 150, above)
- a value for the first term (i.e. line 153)
- an expression for how to generate the nth term from the (n-1)th and (n-2)th terms (i.e. line 158).
So there, you'd implement a Recurrence object with just those definitions for a Chebyshev definite integral, and then you could use other machinery to manipulate them (i.e. loop to evaluate, combine x- and y- integrals to make a 2-d integral). But all that said, I don't think that's worth doing now, given that you've got it working here; I'd want us to think about how to make the APIs there relate a polynomial to its integral (and/or derivatives), and right now there's no precedent for that and hence it's qualitatively new design work, which is always slow.
There was a problem hiding this comment.
Ok. At some point, we can lift this into lsst::geom::polynomials, and refactor this code to use that. I think the tests in here are pretty comprehensive, so they'll help that conversion.
| @@ -224,7 +224,7 @@ class PhotometryTransformChebyshev : public PhotometryTransform { | |||
| * @param[in] bbox The bounding box it is valid within, to rescale it to [-1,1]. | |||
| * @param[in] identity If true, set a_0,0==1, otherwise all coefficients are 0. | |||
| */ | |||
| PhotometryTransformChebyshev(size_t order, afw::geom::Box2D const &bbox, bool identity); | |||
| PhotometryTransformChebyshev(size_t order, lsst::geom::Box2D const &bbox, bool identity); | |||
There was a problem hiding this comment.
The only reason we use lsst::geom instead of just geom:: in afw is because within lsst::afw::, geom:: is ambiguous (because lsst::afw::geom also exists). That's not a problem in lsst::jointcal, so you should just use geom:: here.
There was a problem hiding this comment.
I had thought I was getting warnings at one point when I tried that, but I don't see them now.
| @@ -118,6 +118,17 @@ class ConstrainedPhotometryModel : public PhotometryModel { | |||
| /// Return the initial calibration to use from this photoCalib. | |||
| virtual double initialChipCalibration(std::shared_ptr<afw::image::PhotoCalib const> photoCalib) = 0; | |||
|
|
|||
| /// To hold the return of prepPhotoCalib | |||
| struct PrepPhotoCalib_t { | |||
There was a problem hiding this comment.
The _t suffix here is a pattern that one sees in e.g. the C++ standard library to disambiguate between things and types-of-things. That's not usually a problem for us because we capitalize types. When we need to disambiguate between two types (i.e. a class and a template parameter) our conventions say to use a capital T suffix, but I think even that's not needed here.
|
@TallJimbo : would you mind taking a quick look at the changes I pushed? |
Either I botched the rebase to master, or missed these during DM-13976.
Either I missed these during DM-13976, or I mangled the rebase. I *think* this is the last of them.
Uses the recurrence relation for the integral of Tn(x) to analytically compute the integral. Use the integral to implement a bbox-aware mean()
Refactor to pull out common code between the flux and magnitude versions Add a test of the resulting photoCalib mean post review rename PrepPhotoCalib_t
parejkoj
force-pushed
the
tickets/DM-16305
branch
from
fa9873d
to
a8a3a01
Compare
No description provided.