DM-11905 Convert photoCalib to multiplicative defintion #276

parejkoj · 2017-09-15T21:47:36Z

No description provided.

laurenam · 2017-09-21T21:13:01Z

include/lsst/afw/image/PhotoCalib.h

     *
-     * @see PhotoCalib::computeScaledZeroPoint(), getinstFluxMag0Err()
+     * @see PhotoCalib::computeScaledcalibration(), getCalibrationErr(), getInstFluxMag0()


Capital "C" in computeScaledCalibration() (here and other instances below)?

laurenam · 2017-09-21T21:16:46Z

include/lsst/afw/image/PhotoCalib.h

 * @f]
 * and
 * @f[
- *     2.5/log(10) * sqrt((instFluxErr/instFlux)^2 + (zeroPointErr/zeroPoint)^2) = magnitudeErr
+ *     2.5/log(10) * sqrt((instFluxErr/instFlux)^2 + (calibrationErr/calibration)^2) = magnitudeErr


You are inconsistent here in your use of spaces around operators (I'm not sure what the preference/requirement is for documentation, but consistency would be nice).

Both our C++ and say to put spaces around +-, and prefer no spaces around */. I've made it more consistent with that.

The spaces in the math blocks here don't affect the spacing in the generated html.

laurenam · 2017-09-21T21:20:37Z

include/lsst/afw/image/PhotoCalib.h

-     * @param[in]  instFluxMag0     The constant instFlux/magnitude zero point (instFlux at magnitude 0).
-     * @param[in]  instFluxMag0Err  The error on the zero point.
+     * @param[in]  calibrationMean  The spatially-constant calibration (equal to 1/(instFlux at magnitude 0)).
+     * @param[in]  calibrationError  The error on the zero point.


Extra space after calibrationError

Also, it's a bit confusing to call this the error on the "zero point" since you do not refer to calibrationMean as the "zero point".

You also now seem to vary between Err and Error as the error suffiix (and below with calibrationError vs. _calibrationErr vs. self.calibrationErr vs. getCalibrationErr() vs. instFluxErr)

[and note your commit message says: instFluxMag0Err -> calibrationErr]

Thanks for catching these: I should be using Err everywhere, per RFC-333.

laurenam · 2017-09-21T21:25:41Z

include/lsst/afw/image/PhotoCalib.h

-     * @param[in]  isConstant      Is this PhotoCalib spatially constant?
+     * @param[in]  calibrationMean  The mean of the calibration() over its bounding box.
+     * @param[in]  calibration      The spatially varying photometric zero point.
+     * @param[in]  calibrationError The error on the zero point.


Now here you do refer to calibration as a "zero point". Make all cases consistent (and, if it doesn't seem like overkill, you could emphasize in all these descriptions that it is a multiplicative factor, but I'm not imposing that if you think it's obvious).

Thanks for catching these: I've removed all the erroneous references to "zero point", because that's not a useful term here any more (except the one method that returns instFluxMag0). I'm leaving the mathematical description for the class docstring at the top, so it only has to be written once.

laurenam · 2017-09-21T21:29:43Z

include/lsst/afw/image/PhotoCalib.h

+     * @see PhotoCalib::computeScaledcalibration(), getCalibrationMean()
+     *
+     * @returns     The instFlux magnitude zero point.
+     */


This documentation looks wrong (you should be describing getting an error)

I believe this one is correct: getInstFluxMag0() returns an inverse calibration (the magnitude zero point), not an error. But my above inconsistencies regarding zero point and Err probably didn't help matters.

laurenam · 2017-09-21T21:33:06Z

python/lsst/afw/image/photoCalib.cc

-                                         PhotoCalib::instFluxToMaggies,
+    cls.def("instFluxToMaggies",
+            (double (PhotoCalib::*)(double, afw::geom::Point<double, 2> const &) const) &
+                    PhotoCalib::instFluxToMaggies,


Purely formatting changes should be on their own commit.

I'm using an "auto clang-format on save" plugin in SublimeText, and it apparently prefers the latter. I'm not going to argue with it (the & is right up on the 110 character boundary, which is probably the issue).

I'm not questioning the formatting choices, just saying they should be on their own commit (you can do an initial "auto clang-format on save" before making any actual code changes to the file to keep them isolated).

I agree with you in principle and will try to watch for other clang-format changes in the future, but I don't think it's worth cherry picking this out in this case.

laurenam · 2017-09-22T17:02:16Z

include/lsst/afw/image/PhotoCalib.h

- * The spatially varying flux/magnitude zero point is defined such that,
- * at a position (x,y) in the domain of the boundedField zeroPoint
+ * The spatially varying flux/magnitude zero point has units of maggies/ADU, and is defined such that,
+ * at a position (x,y) in the domain of the boundedField calibration


I assume (x, y) here is taken as the object's (slot) centroid?

The line break here seems unnatural.

(x,y) is whatever position you pass in to whichever of the overloaded functions. The functions that take Record use get('x') as documented.

Reflowed the line: it's just under the 110 character limit.

laurenam · 2017-09-28T20:15:58Z

python/lsst/afw/image/photoCalib.cc

-                                         PhotoCalib::instFluxToMaggies,
+    cls.def("instFluxToMaggies",
+            (double (PhotoCalib::*)(double, afw::geom::Point<double, 2> const &) const) &
+                    PhotoCalib::instFluxToMaggies,


I'm not questioning the formatting choices, just saying they should be on their own commit (you can do an initial "auto clang-format on save" before making any actual code changes to the file to keep them isolated).

laurenam · 2017-09-28T20:17:03Z

include/lsst/afw/image/PhotoCalib.h

     *
     * This value is defined such that for some instFluxErr, instFlux, and flux:
-     *     sqrt((instFluxErr/instFlux)^2 + (zeroPointErr/zeroPoint(x,y))^2)*flux = fluxErr (in maggies)
+     *     sqrt((instFluxErr/instFlux)^2 + (calibrationErr/calibration(x,y))^2)*flux = fluxErr (in maggies)


I'm still seeing spacing inconsistencies: here vs. 312 vs. 337, vs...

I've cleaned up all of these to make them proper math blocks, so they'll render properly in the final output, and also tried to make them consistent with our C++ guidelines.

laurenam · 2017-09-28T20:18:22Z

src/image/PhotoCalib.cc

-                      schema.addField<double>("instFluxMag0Err", "1-err error on instFluxmag0", "count")),
+              calibrationMean(schema.addField<double>(
+                      "calibrationMean", "mean calibration on this PhotoCalib's domain", "count")),
+              calibrationErr(schema.addField<double>("calibrationErr", "1-sigma err error on calibrationMean",


"1-sigma err error"?!

Rename several things: instFluxMag0 -> calibrationMean zeroPoint() -> calibration() instFluxMag0Err -> calibrationErr Remove getInstFluxMag0Err Fix bug in test, where it was using self.instFlux0Err instead of the passed instFlux0Err (now calibrationErr). This bug was only noticeable when I was testing with non-physical errors (e.g. >10x calibration); I've checked the rest of the code for similar bugs, and found none.

parejkoj force-pushed the tickets/DM-11905 branch 4 times, most recently from 77be4f7 to d6e1de3 Compare September 19, 2017 00:52

laurenam reviewed Sep 22, 2017

View reviewed changes

laurenam reviewed Sep 28, 2017

View reviewed changes

parejkoj force-pushed the tickets/DM-11905 branch 3 times, most recently from 34f6c47 to bff48c2 Compare October 4, 2017 21:05

parejkoj force-pushed the tickets/DM-11905 branch from 069ca2a to 7237e73 Compare October 10, 2017 18:59

parejkoj merged commit 7237e73 into master Oct 10, 2017

ktlim deleted the tickets/DM-11905 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-11905 Convert photoCalib to multiplicative defintion #276

DM-11905 Convert photoCalib to multiplicative defintion #276

parejkoj commented Sep 15, 2017

laurenam Sep 21, 2017

laurenam Sep 21, 2017

parejkoj Sep 26, 2017

laurenam Sep 21, 2017

parejkoj Sep 26, 2017

laurenam Sep 21, 2017

parejkoj Sep 26, 2017

laurenam Sep 21, 2017

parejkoj Sep 26, 2017

laurenam Sep 21, 2017

parejkoj Sep 26, 2017

laurenam Sep 28, 2017

parejkoj Oct 4, 2017

laurenam Sep 22, 2017

parejkoj Sep 26, 2017

laurenam Sep 28, 2017

laurenam Sep 28, 2017

parejkoj Oct 4, 2017

laurenam Sep 28, 2017

DM-11905 Convert photoCalib to multiplicative defintion #276

DM-11905 Convert photoCalib to multiplicative defintion #276

Conversation

parejkoj commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment