Change doc of used field to calib_photometryUsed. Add reserved and c… #119

pgee2000 · 2017-04-25T04:49:32Z

…andidate flags.

Flag marks sources used in final calibration, name changed from photometryStandard.

Added the ability to reserve a fraction of candidates selected from the initial match.
Mark those calib_photometryReserved.

Mark the remainder of the candidates calib_photometryCandidate.

parejkoj

A few minor changes, but my bigger concern is the lack of a unittest of these new features. I think that's a requirement before we can merge this.

parejkoj · 2017-04-27T21:25:44Z

python/lsst/pipe/tasks/photoCal.py

@@ -294,10 +306,16 @@ def __init__(self, refObjLoader, schema=None, **kwds):
        self.scatterPlot = None
        self.fig = None
        if self.config.doWriteOutput:
-            self.outputField = schema.addField("photocal_photometricStandard", type="Flag",
+            self.usedKey = schema.addField("calib_photometryUsed", type="Flag",
+                                               doc="set if source was used in photometric calibration")


wonky indentation

parejkoj · 2017-04-27T21:27:24Z

python/lsst/pipe/tasks/photoCal.py

@@ -648,21 +670,42 @@ def run(self, exposure, sourceCat):
            frame = None

        res = self.loadAndMatch(exposure, sourceCat)
+
+        #from res.matches, reserve a fraction of the population and mark the sources as reserved
+        reserveList = []


move reserveList inside the if, since we don't need it unless we're reserving things.

parejkoj · 2017-04-27T21:29:26Z

python/lsst/pipe/tasks/photoCal.py

+            reserveList = random.sample(res.matches,
+                                        int((self.config.reserveFraction)*len(res.matches)))
+
+            for cand in reserveList:


I'd rather that iterator be candidate or reserved

parejkoj · 2017-04-27T21:30:03Z

python/lsst/pipe/tasks/photoCal.py

+                res.matches.remove(cand)
+
+            if reserveList and self.reservedKey is not None:
+                for cand in reserveList:


Again, candidate or reserved

parejkoj · 2017-05-04T20:27:27Z

tests/testPhotoCal.py

+        assert(cat.get("calib_photometryUsed").sum() <= candidates)
+
+        #   set the reserve fraction, and see if the right proportion are reserved.
+        self.config.doWriteOutput = True


This usage here strongly suggests that doWriteOutput is the wrong name for that option. If you don't want to change it on this ticket, please file another ticket to change it in the future.

parejkoj

See comments.

In addition, please either set your editor up to use a linter inline (ask on #software-dev on slack for help setting it up), or run flake8 before pushing.

parejkoj · 2017-05-08T18:51:59Z

python/lsst/pipe/tasks/photoCal.py

-        if self.config.doWriteOutput:
-            self.outputField = schema.addField("photocal_photometricStandard", type="Flag",
-                                               doc="set if source was used in photometric calibration")
+        if not schema is None:


schema is not None:

https://www.python.org/dev/peps/pep-0008/#programming-recommendations
https://developer.lsst.io/coding/python_style_guide.html#is-and-is-not-should-be-used-when-comparing-to-none

parejkoj · 2017-05-08T18:57:08Z

python/lsst/pipe/tasks/photoCal.py

+            self.usedKey = schema.addField("calib_photometryUsed", type="Flag",
+                                           doc="set if source was used in photometric calibration")
+            self.candidateKey = schema.addField("calib_photometryCandidate", type="Flag",
+                                                doc="set if source was used in photometric calibration")


Maybe doc should be: "set if source was a possible candidate for photometric calibration"

parejkoj · 2017-05-08T18:59:18Z

python/lsst/pipe/tasks/photoCal.py

+            self.candidateKey = schema.addField("calib_photometryCandidate", type="Flag",
+                                                doc="set if source was used in photometric calibration")
+            self.reservedKey = schema.addField("calib_photometryReserved", type="Flag",
+                                               doc="set if source was reserved in photometric calibration")


maybe reword doc: "set if source was reserved, and thus not used, in photometric calibration"

parejkoj · 2017-05-08T21:30:07Z

tests/testPhotoCal.py

+        self.config.reserveFraction = .3
+        task.run(exposure=self.exposure, sourceCat=cat)
+        reserved = cat.get("calib_photometryReserved").sum()
+        assert(reserved == int(.3 * candidates))


Please use self.assertEqual(reserved, int(.3*candidates))

parejkoj · 2017-05-08T21:31:01Z

tests/testPhotoCal.py

+        task.run(exposure=self.exposure, sourceCat=cat)
+        reserved = cat.get("calib_photometryReserved").sum()
+        assert(reserved == int(.3 * candidates))
+        assert(cat.get("calib_photometryUsed").sum() <= (candidates - reserved))


As above, self.assertLessEqual(cat.get(...).sum(), (candidates - reserved))

parejkoj · 2017-05-08T21:31:44Z

tests/testPhotoCal.py

+        #   test that by default, no stars are reserved and used < candidates
+        task.run(exposure=self.exposure, sourceCat=cat)
+        candidates = cat.get("calib_photometryCandidate").sum()
+        assert(cat.get("calib_photometryReserved").sum() == 0)


self.assertEqual(cat.get..., 0)

parejkoj · 2017-05-08T21:32:10Z

tests/testPhotoCal.py

+        task.run(exposure=self.exposure, sourceCat=cat)
+        candidates = cat.get("calib_photometryCandidate").sum()
+        assert(cat.get("calib_photometryReserved").sum() == 0)
+        assert(cat.get("calib_photometryUsed").sum() <= candidates)


self.assertLessEqual(cat.get().sum(), candidates)

Though actually, I'd think you'd be able to know exactly how many objects are selected for used here: can't you test with that number, so we can know if it ever changes?

I don't understand how I could know this value except by running the calibration and noting the number. But that's circular.
Also, I think this is just a test that the flag writing mechanism works. To test whether the number is correct seems or changes over time seems like it is a different problem.

Running the test once to get the "current" value and then testing on that value is a reasonable approach, but I agree that it's probably better as a separate test. Probably best done as part of SQUASH, actually, once that's up and running.

parejkoj · 2017-05-09T18:46:02Z

python/lsst/pipe/tasks/photoCal.py

            self.usedKey = schema.addField("calib_photometryUsed", type="Flag",
                                           doc="set if source was used in photometric calibration")
            self.candidateKey = schema.addField("calib_photometryCandidate", type="Flag",
-                                                doc="set if source was used in photometric calibration")
+                                                doc="set if source was as possible candidate in photometric calibration")


"was a possible"

Flag marks sources using calib_photometryUsed flag: changed from photometryStandard. Add feature to reserve a fraction of candidates selected from the initial match. Mark those calib_photometryReserved. Mark the remainder of the candidates calib_photometryCandidate. Unit test added to testPhotoCal.py

parejkoj requested changes Apr 27, 2017

View reviewed changes

pgee2000 force-pushed the tickets/DM-9050 branch from 10e8f58 to e0b9233 Compare May 3, 2017 05:29

parejkoj reviewed May 4, 2017

View reviewed changes

pgee2000 force-pushed the tickets/DM-9050 branch 2 times, most recently from 3edf773 to acdf3a4 Compare May 8, 2017 16:58

parejkoj reviewed May 8, 2017

View reviewed changes

parejkoj reviewed May 9, 2017

View reviewed changes

parejkoj approved these changes May 9, 2017

View reviewed changes

pgee2000 force-pushed the tickets/DM-9050 branch 2 times, most recently from 3b583c8 to d8371bf Compare May 10, 2017 01:18

pgee2000 merged commit d8371bf into master May 10, 2017

ktlim deleted the tickets/DM-9050 branch August 25, 2018 06:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change doc of used field to calib_photometryUsed. Add reserved and c… #119

Change doc of used field to calib_photometryUsed. Add reserved and c… #119

pgee2000 commented Apr 25, 2017

parejkoj left a comment

parejkoj Apr 27, 2017

pgee2000 May 3, 2017

parejkoj Apr 27, 2017

pgee2000 May 3, 2017

parejkoj Apr 27, 2017

pgee2000 May 3, 2017

parejkoj Apr 27, 2017

pgee2000 May 3, 2017

parejkoj May 4, 2017

parejkoj left a comment •

edited

parejkoj May 8, 2017

pgee2000 May 8, 2017

parejkoj May 8, 2017

pgee2000 May 8, 2017

parejkoj May 8, 2017

pgee2000 May 8, 2017

parejkoj May 8, 2017

pgee2000 May 8, 2017

parejkoj May 8, 2017

parejkoj May 8, 2017

pgee2000 May 8, 2017

parejkoj May 8, 2017

parejkoj May 8, 2017

pgee2000 May 8, 2017

parejkoj May 9, 2017

parejkoj May 9, 2017

Change doc of used field to calib_photometryUsed. Add reserved and c… #119

Change doc of used field to calib_photometryUsed. Add reserved and c… #119

Conversation

pgee2000 commented Apr 25, 2017

parejkoj 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

parejkoj left a comment • 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

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

parejkoj left a comment •

edited