DM-14102 makePsfCandidates is now its own Task #118

parejkoj · 2018-04-27T22:12:08Z

No description provided.

mrawls

Overall this looks good. Please see the individual comments.

mrawls · 2018-05-03T19:03:34Z

python/lsst/meas/algorithms/starSelector.py

+
+            - starCat  catalog of stars that were selected as stars and successfuly made into PSF candidates
+                        (a subset of sourceCat whose records are shallow copies)
+            - psfCandidates  list of PSF candidates (lsst.meas.algorithms.PsfCandidate)


The result struct no longer contains psfCandidates, just starCat

mrawls · 2018-05-03T19:03:37Z

python/lsst/meas/algorithms/starSelector.py

                (an lsst.afw.table.ReferenceMatchVector), or None. Some star selectors
                will ignore this argument, others may require it. See the usesMatches class variable.
-        @param[in] isStarField  name of flag field to set True for stars, or None to not set a field;
+        isStarField :


Yay numpydoc! Please finish these up nicely by adding what flavor of object each parameter is. You may have bonus points if you finish converting the rest of the docstrings in here to numpydoc.

mrawls · 2018-05-03T19:03:43Z

python/lsst/meas/algorithms/starSelector.py

@@ -20,8 +20,6 @@
 # the GNU General Public License along with this program.  If not,
 # see <https://www.lsstcorp.org/LegalNotices/>.
 #
-from __future__ import absolute_import, division, print_function
-


This should technically be in a separate de-python2-ification commit

mrawls · 2018-05-03T19:03:48Z

python/lsst/meas/algorithms/makePsfCandidates.py

+
+            vmax = afwMath.makeStatistics(im, afwMath.MAX).getValue()
+            if not np.isfinite(vmax):
+                continue


What is this vmax business used for? Again, I realize it was in the original code but it's not clear why.

We think it is a way of checking if the candidate cutout image doesn't contain NaN or inf. Whether this is the best approach for that, I don't know. Not knowing more, I'd rather not attempt to clarify it in place.

mrawls · 2018-05-03T19:03:52Z

python/lsst/meas/algorithms/makePsfCandidates.py

+                # The setXXX methods are class static, but it's convenient to call them on
+                # an instance as we don't know exposures's pixel type
+                # (and hence psfCandidate's exact type)
+                if not didSetSize:


You set didSetSize to False right before this loop, so I'm confused how it could ever be True (and therefore skip this if-block). I realize this was in the original code but I still want to understand it.

psfCandidate and its parent class SpatialCellImageCandidate, inappropriately use static members for border, width, and height (among other things), meaning once you set them, they apply to all psfCandidates. We have to make those not static if we want to clean up this block. That's going to require an RFC, as according to @PaulPrice there is some code that assumes they are static.

mrawls · 2018-05-03T19:08:54Z

python/lsst/meas/algorithms/makePsfCandidates.py

+            ``lsst.meas.algorithms.starSelector.run()``.
+        exposure : `lsst.afw.image.Exposure` (optional)
+            The exposure containing the sources.
+


Add isStarField to the docstring here. The name suggests to me it's a Boolean flag, but defaulting to None is weird. I'd prefer to have it True/False throughout.

Good point. I've renamed it starSelectedField, since it's actually str, not a bool.

This is, however, reminding me that we don't have a unittest for makePsfCandidates...

Great idea! How about a unit test! 😸

Based on my work on the new test, I've renamed it again to psfCandidateField, which I think is even better. And there's a new unittest in another commit.

mrawls · 2018-05-03T19:09:01Z

python/lsst/meas/algorithms/makePsfCandidates.py

+
+
+class MakePsfCandidatesTask(pipeBase.Task):
+    """Create PSF candiates given an input catalog.


Typo: candidates

mrawls · 2018-05-03T19:09:10Z

tests/test_psfSelectTest.py

@@ -298,7 +300,8 @@ def testPsfCandidate(self):

        # select psf stars
        print("PSF selection")
-        psfCandidateList = self.starSelector.run(exposDist, sourceList).psfCandidates
+        stars = self.starSelector.run(exposDist, sourceList)
+        psfCandidateList = self.makePsfCandidates.run(stars.starCat, exposure=exposDist).psfCandidates

        # determine the PSF
        print("PSF determination")


Should this test really have a bunch of print statements floating around?

No, it shouldn't. But there are so many janky things about this test, I don't want to touch it other than to get it to work with the new API.

mrawls · 2018-05-03T19:33:03Z

I also noticed this is failing Travis, but I'm not sure whether that is meaningful or not since we're still in the process of getting Travis set up across the Stack.

timj · 2018-05-03T19:37:31Z

There is a clash with #119 -- @r-owen enabled travis to test that PR but now travis runs for all pull requests. #119 seems like a big change so I hope it doesn't conflict with this PR. Not sure when it will be merged.

r-owen · 2018-05-03T19:45:11Z

I enabled Travis, but did not set it to block merges of failing code. Once I merge DM-14253 we can require the Travis check to pass in order to merge to master. Just waiting on review.

parejkoj · 2018-05-04T00:57:01Z

I've pushed a cleanup commit (no review needed) based on your comments and a new test of MakePsfCandidatestTask. Can you please review the new test? It's not perfect (doesn't test the max==NaN/inf part of the code, but I couldn't figure out how to make fake sources with NaNs in them), but it at least tests two code paths, which is more than we had before.

mrawls · 2018-05-04T20:01:00Z

tests/test_psfCandidate.py

@@ -37,46 +36,96 @@
    display = False


+def makeEmptyCatalog(psfCandidateField=''):


Is there a reason you're using '' as the default rather than just None?

mrawls · 2018-05-04T20:01:05Z

tests/test_psfCandidate.py

+            found = True
+            print(fp.getBBox())
+            break
+    assert found, "Unable to find central peak in footprint: faulty test"


It's not immediately clear that this line is asserting found must be True and prints the "unable to find..." message if it's False. But if this is the correct/pythonic way to do so, I can live with it.

This is the syntax for the python assert statement. Is the purpose of the assert here clear?

Would you be happier with assert found is True, "sometext" instead?

I think it's really a question of it looking different because everywhere else in tests you see self.assertTrue(found) but here you see a bare assert because self has not been passed in to the function.

If this was in a class that the other test classes inherited from (maybe with multiple inheritance) then the standard scheme could be used.

In this case, it's supposed to be "is my test even sane?", hence the raw assert.

I've added a note to clarify the purpose.

Please don't use foo is True; it isn't the same as testing the boolean value of foo and it's not good Python unless you are sure you need exactly the True singleton.

Oh, right @r-owen I forgot about that. Anyway, I think it should be clear with the comment I added.

mrawls · 2018-05-04T20:01:11Z

tests/test_psfCandidate.py

@@ -139,6 +188,48 @@ def testFaintNeighborMasking(self):
        self.checkCandidateMasking([(self.x+5, self.y, 0.5)], threshold=0.9, pixelThreshold=1.0)


+class MakePsfCandiatesTaskTest(lsst.utils.tests.TestCase):


Probably don't want a typo in the class name ("Candidates") 😄

Clearly I cannot type that word.

mrawls · 2018-05-04T20:01:15Z

tests/test_psfCandidate.py

+        self.badIds = [1, ]
+        self.goodIds = [2, 3]
+        # x and y coordinate: keep these in sync with the above good/bad list.
+        self.xx = [0, 100, 200]


xcoord and ycoord might be nicer variable names than xx and yy

mrawls · 2018-05-04T20:02:32Z

Thanks for adding the test! It looks fine to me.

Fix existing tests to use new MakePsfCandidatesTask following StarSelectorTask. Add tests for MakePsfCandidates and cleanup existing psfCandidate tests. * Pull out the catalog and false source creation to use in both tests. * Refactor `CandidateMaskingTestCase.createCandidate()` to call `makePsfCandidate` instead of the raw constructor, since that is what is most commonly used elsewhere. * Rename self.exp->self.exposure in CandidateMaskingTestCase.

parejkoj force-pushed the tickets/DM-14102 branch 4 times, most recently from 317b4ae to ce1bb65 Compare May 1, 2018 20:47

mrawls reviewed May 3, 2018

View reviewed changes

parejkoj force-pushed the tickets/DM-14102 branch 3 times, most recently from d55ac03 to 91abe95 Compare May 4, 2018 00:49

mrawls reviewed May 4, 2018

View reviewed changes

parejkoj force-pushed the tickets/DM-14102 branch from 01ca025 to e1d4f34 Compare May 4, 2018 21:19

parejkoj force-pushed the tickets/DM-14102 branch from e1d4f34 to e6952d2 Compare May 4, 2018 21:27

parejkoj merged commit 113cadf into master May 4, 2018

ktlim deleted the tickets/DM-14102 branch August 25, 2018 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-14102 makePsfCandidates is now its own Task #118

DM-14102 makePsfCandidates is now its own Task #118

parejkoj commented Apr 27, 2018

mrawls left a comment

mrawls May 3, 2018

mrawls May 3, 2018

mrawls May 3, 2018

mrawls May 3, 2018

parejkoj May 3, 2018

mrawls May 3, 2018

parejkoj May 3, 2018 •

edited

mrawls May 3, 2018

parejkoj May 3, 2018

mrawls May 3, 2018

parejkoj May 4, 2018

mrawls May 3, 2018

mrawls May 3, 2018

parejkoj May 3, 2018

mrawls commented May 3, 2018

timj commented May 3, 2018

r-owen commented May 3, 2018

parejkoj commented May 4, 2018

mrawls May 4, 2018

mrawls May 4, 2018

parejkoj May 4, 2018

timj May 4, 2018

timj May 4, 2018

parejkoj May 4, 2018

parejkoj May 4, 2018

r-owen May 4, 2018

parejkoj May 4, 2018

mrawls May 4, 2018

parejkoj May 4, 2018

mrawls May 4, 2018

mrawls commented May 4, 2018



		class MakePsfCandidatesTask(pipeBase.Task):
		"""Create PSF candiates given an input catalog.

		@@ -37,46 +36,96 @@
		display = False


		def makeEmptyCatalog(psfCandidateField=''):

		@@ -139,6 +188,48 @@ def testFaintNeighborMasking(self):
		self.checkCandidateMasking([(self.x+5, self.y, 0.5)], threshold=0.9, pixelThreshold=1.0)


		class MakePsfCandiatesTaskTest(lsst.utils.tests.TestCase):

DM-14102 makePsfCandidates is now its own Task #118

DM-14102 makePsfCandidates is now its own Task #118

Conversation

parejkoj commented Apr 27, 2018

mrawls 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 May 3, 2018 • edited

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

mrawls commented May 3, 2018

timj commented May 3, 2018

r-owen commented May 3, 2018

parejkoj commented May 4, 2018

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

mrawls commented May 4, 2018

parejkoj May 3, 2018 •

edited