DM-14153 count measured and reference stars after outlier rejection #82

parejkoj · 2018-04-19T23:21:15Z

No description provided.

r-owen

Please add a unit test for countStars

I am very uneasy about your first commit and suggest ditching it. I agree it simplifies the call signature of many methods, but the cost is that your task now has state, which is a violation of our task guidelines http://doxygen.lsst.codes/stack/doxygen/x_masterDoxyDoc/pipe_tasks_write_task.html

r-owen · 2018-04-23T20:07:18Z

python/lsst/jointcal/jointcal.py

+    def check_stars(self):
+        """Count measured and reference stars per ccd and warn/log them."""
+        for ccdImage in self.associations.getCcdImageList():
+            stars = ccdImage.countStars()


I strongly suggest unpacking the returned value into nMeasStars and nRefStars or similar. I think it would make the code clearer, because the name stars sounds like a collection of star objects, not a count or pair of counts, and stars gives no hint as to which component is which.

r-owen · 2018-04-23T20:09:12Z

include/lsst/jointcal/CcdImage.h

+    /**
+     * Count the number of valid measured and reference stars that fall within this ccdImage.
+     *
+     * @return (measured, reference) stars in the image.


Consider adding "number of" before (measured", just to emphasize that this is a pair of counts, not a pair of collections of star objects. It duplicates the brief description, so I certainly won't insist.

r-owen · 2018-04-23T20:12:57Z

python/lsst/jointcal/jointcal.py

+            if stars[1] < self.config.minRefStarsPerCcd:
+                self.log.warn("ccdImage %s has only %s RefStars (desired %s)",
+                              ccdImage.getName(), stars[1], self.config.minRefStarsPerCcd)
+


Can you do more than just log a warning if a count is too low? Is there a threshold for either value below which the fit should give up (e.g. one or two additional config parameters)?

That is a very good question. I think I'd rather start by issuing warnings now, and turn it into a raise or equivalent after we've gotten a sense of how often it triggers. Right now, I just want to know about it as part of debugging things, and I don't really have a good sense for what an appropriately "safe" value is a priori. @TallJimbo might have opinions there, though?

It's probably possible to come up with a minimum for at least some specific models from first principles, but I think it's probably most productive to just log things for now.

r-owen · 2018-04-23T20:15:12Z

src/CcdImage.cc

+std::pair<int, int> CcdImage::countStars() const {
+    int measuredStars = 0;
+    int refStars = 0;
+    for (auto &measuredStar : _catalogForFit) {


please insert a const after auto since the code does not modify _catalogForFit

parejkoj · 2018-04-23T22:56:21Z

I've reverted the "add associations to jointcal instance" commit and reworked the other changes to operate by passing associations around.

I wanted to add a test for check_stars(), but I'm not sure how to go about it: all it does is send a log.warn message, and I don't know how to capture that in a test to check it.

r-owen

Overall looks great. Some nice improvements here. The new test utilities seem helpful.

A few minor requests for changes.

r-owen · 2018-04-30T20:47:16Z

bin.src/plot_photoCalib.py

@@ -140,7 +140,7 @@ def makePhotoCalibImages(visit, butler, step=8, chips=[], tract=None,
    return images


-class ImageMaker(object):
+class ImageMaker():


Please consider deleting the () as well, here and in other class definitions with no parent class or metaclass. I think removing unnecessary punctuation adds a bit of clarity. On the other hand flake8 apparently accepts this code, so feel free to ignore this suggestion.

I'd tend to agree that it's cleaner to remove the parens.

I didn't even know I could! I definitely agree.

r-owen · 2018-04-30T20:51:04Z

include/lsst/jointcal/Associations.h

@@ -24,6 +24,8 @@
 namespace lsst {
 namespace jointcal {

+using refFluxMapType = std::map<std::string, std::vector<double>>;


This is a type so please capitalize the name, e.g. RefFluxMapType or even RefFluxMap

r-owen · 2018-04-30T20:56:59Z

include/lsst/jointcal/Associations.h

@@ -74,8 +91,7 @@ class Associations {
     * @param[in]  ccd        The ccd identifier
     * @param[in]  control    The JointcalControl object
     */
-    void addImage(lsst::afw::table::SortedCatalogT<lsst::afw::table::SourceRecord> &catalog,
-                  std::shared_ptr<lsst::afw::geom::SkyWcs> wcs,
+    void addImage(afw::table::SourceCatalog &catalog, std::shared_ptr<lsst::afw::geom::SkyWcs> wcs,


Consider renaming this to createCcdImage or something similar; it may reduce the confusion caused by having a constructor that takes a list of CcdImage.

r-owen · 2018-04-30T20:57:46Z

include/lsst/jointcal/Gtransfo.h

@@ -59,7 +59,7 @@ class Gtransfo {
     * Transform a bounding box, taking either the inscribed or circumscribed box.
     *
     * @param[in] inputframe The frame to be transformed.
-     * @param[in] which Return the inscribed (true) or circumscribed (false) box.
+     * @param[in] inscribed Return the inscribed (true) or circumscribed (false) box.


r-owen · 2018-04-30T21:06:29Z

include/lsst/jointcal/Associations.h

+    /**
+     * Sets a shared tangent point for all ccdImages, using the mean of the centers of all ccdImages.
+     */
+    void setCommonTangentPoint();


Consider a name such computeCommonTangentPoint. It is less confusing than a set method that takes no arguments (usually set takes the value to be set). Plus less overloading makes the Python easier to understand.

r-owen · 2018-04-30T21:18:52Z

python/lsst/jointcal/testUtils.py

+       Result struct with components:
+
+       - `catalog` : Catalogs containing fake sources
+           (`lsst.afw.table.SourceCatalog`).


Please swap the type and the description here and for ccdImage immediately below and clean up bbox immediately after that. I guess it's not a Parameters list and maybe it doesn't matter, but it's a bit odd being different than parameter lists for no readily apparently reason (and different item by item).

That's how Struct returns are supposed to be documented: https://developer.lsst.io/python/numpydoc.html#struct-return-types

Oh, and the reason bbox looks different is because it's on the same line.

Thanks. I didn't know about that recommendation

r-owen · 2018-04-30T21:20:23Z

python/lsst/jointcal/testUtils.py

+    visitInfo = butler.get("calexp_visitInfo", dataId=dataId)
+    bbox = butler.get('calexp_bbox', dataId=dataId)
+    detector = butler.get('calexp_detector', dataId=dataId)
+    filt = butler.get("calexp_filter", dataId=dataId).getName()


At risk of being too picky, please consider being uniform in your use of single quotes or double quotes for these strings. In the 5 lines above you have 2 one way and 3 the other. It works, and "a foolish consistency" and all, so ignore if you prefer.

r-owen · 2018-04-30T21:28:13Z

python/lsst/jointcal/testUtils.py

+    ----------
+    catalog : `lsst.afw.table.SourceCatalog`
+        The table to get sources from.
+    pixToFocal : `lsst.afw.geom.Transform`


I think you mean lsst.afw.geom.TransformPoint2ToPoint2

Transform is not actually a class in Python (though it is a templated class in C++)

r-owen · 2018-04-30T21:36:49Z

include/lsst/jointcal/CcdImage.h

+    /**
+     * Count the number of valid measured and reference stars that fall within this ccdImage.
+     *
+     * @return Number of (measured, reference) stars in the image.


please add a few words explaining whether reference stars that are associated with an invalid measured star are counted or excluded (with some sort of justification, if practical; it may not be worth the trouble, since this count is only intended as a simple diagnostic measure and it may not be clear yet -- or ever -- which option is preferable).

r-owen · 2018-04-30T21:40:35Z

tests/test_ccdImage.py

+        self.associations.setCommonTangentPoint()
+        self.associations.associateCatalogs(matchCut)
+        self.assertEqual(measStars, nStars)
+        self.assertEqual(refStars, 0)


How can one tell that calling setCommonTangentPoint and associateCatalogs did anything?

If they don't raise an exception, I guess.

An offline conversation cleared this point up: I've added a couple of sanity checks around this block to show what's happening.

Moving cfht_minimal out of testdata_jointcal, because it's small (~2MB), and it's useful to have a minimally useful dataset for tests that don't require e.g. refcats or full sized real catalogs. fix test_jointcal_cfht_minimal.py to reflect new dataset location. Add flake8 ignore for test data.

A short block of python was starting to proliferate that computed a common tangent point from the ccdImages. That code might as well just live in associations as an argument-less `computeCommonTangentPoint()`. `refFluxMap` in collectRefStars() can take an empty dict, so it might as well have default values for that.

Compare countStars() output with a configurable "minimum" number of measured and reference stars per ccdImage after every outlier rejection loop.

Added test_ccdImage.py with tests of countStars(). Initial version of testUtils came from test_photometryModel.py, which now uses the new code, and now tests many points as part of test_toPhotoCalib().

We have to create the ccdImage before we can add it to the list, and a reviewer suggested this would make that clearer.

r-owen requested changes Apr 23, 2018

View reviewed changes

parejkoj force-pushed the tickets/DM-14153 branch 3 times, most recently from 716f12a to 1346822 Compare April 27, 2018 01:13

r-owen approved these changes Apr 30, 2018

View reviewed changes

parejkoj added 9 commits April 30, 2018 17:11

Expand star pybind11 to better support tests

dcddd4e

Add ccdImageList constructor for Associations.

97152fb

Add ccdImage.countStars()

66dd709

Compare countStars() output with a configurable "minimum" number of measured and reference stars per ccdImage after every outlier rejection loop.

Add testUtils for building fake catalogs; use it in tests.

9f2aa92

Added test_ccdImage.py with tests of countStars(). Initial version of testUtils came from test_photometryModel.py, which now uses the new code, and now tests many points as part of test_toPhotoCalib().

Fix doxygen missing parameter warning

9834c91

Remove __future__ and builtins (no more py2 support!)

d1de1ba

Rename addImage->createCcdImage

9b47467

We have to create the ccdImage before we can add it to the list, and a reviewer suggested this would make that clearer.

parejkoj force-pushed the tickets/DM-14153 branch from 3737af9 to 9b47467 Compare May 1, 2018 00:23

parejkoj merged commit 11095dd into master May 1, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-14153 count measured and reference stars after outlier rejection #82

DM-14153 count measured and reference stars after outlier rejection #82

parejkoj commented Apr 19, 2018

r-owen left a comment

r-owen Apr 23, 2018

r-owen Apr 23, 2018

r-owen Apr 23, 2018

parejkoj Apr 23, 2018

TallJimbo Apr 25, 2018

r-owen Apr 23, 2018

parejkoj commented Apr 23, 2018

r-owen left a comment

r-owen Apr 30, 2018

timj Apr 30, 2018

parejkoj Apr 30, 2018

r-owen Apr 30, 2018

r-owen Apr 30, 2018

r-owen Apr 30, 2018

r-owen Apr 30, 2018

r-owen Apr 30, 2018

parejkoj Apr 30, 2018

parejkoj Apr 30, 2018

r-owen Apr 30, 2018

r-owen Apr 30, 2018

r-owen Apr 30, 2018

r-owen Apr 30, 2018

r-owen Apr 30, 2018

parejkoj Apr 30, 2018

parejkoj May 1, 2018

DM-14153 count measured and reference stars after outlier rejection #82

DM-14153 count measured and reference stars after outlier rejection #82

Conversation

parejkoj commented Apr 19, 2018

r-owen left a 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

parejkoj commented Apr 23, 2018

r-owen left a 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