Ticket #2057 (closed enhancement: fixed)

Opened 8 years ago

Last modified 8 years ago

Add support for applying colour terms to the reference magnitudes

Reported by: rhl Owned by: rhl
Priority: normal Milestone:
Component: meas_photocal Keywords:
Cc: rhl, cloomis, price Blocked By:
Blocking: Project: LSST
Version Number: 5.0
How to repeat:

not applicable

Description

In general, the catalogue magnitudes used to photometrically calibrate data will not be on the natural photometric system of the camera we're processing. Please add a way to apply colour terms to the input catalogues.

Attachments

testPhotoCal.diff (864 bytes) - added by krughoff 8 years ago.
Diff of fix to tests/testPhotoCal.py

Change History

comment:1 Changed 8 years ago by DefaultCC Plugin

  • Cc rhl, cloomis, price added

comment:2 Changed 8 years ago by rhl

  • Status changed from new to assigned
  • Component changed from meas_astrom to meas_photocal

comment:3 Changed 8 years ago by rhl

  • Status changed from assigned to inTicketWork

comment:4 Changed 8 years ago by rhl

  • Owner changed from rhl to krughoff
  • Status changed from inTicketWork to inStandardsReview

I've added the functionality (it relies on #2042) and tested it with SuprimeCam data (and added a unit test). 52ecc04

comment:5 Changed 8 years ago by krughoff

  • Owner changed from krughoff to rhl
  • Status changed from inStandardsReview to inTicketWork

Review comments:

  1. The change to the run method in PhotoCalTask causes a test to fail. I'll attach the diff of a fix. I can push my change if you'd like.
  2. I was not familiar with this idiom and I found the recursiveness of the design to be somewhat confusing to begin with. After looking at the test it's clear how it should work. My concern is that the added complexity of dealing with several devices at the same time may not be necessary for the most common use case: a single device type with several filters.
  3. I did not see an obvious place for the _colorterms dictionary to be set (via Colerterm.setColorterms()) in PhotoCalTask. Is this intended to be done by sub-classing the task?

The only blocker to merging, in my opinion, is 1.

Changed 8 years ago by krughoff

Diff of fix to tests/testPhotoCal.py

comment:6 follow-up: ↓ 9 Changed 8 years ago by rhl

  • Status changed from inTicketWork to closed
  1. I applied the patch, sorry, and thank you. Another strategy would have been to create a branch of your own and commit the changes (so I could just merge them), but this way works fine too. One advantage of the branch approach is that your name would have appeared on the fixes. Asking you to push your changes would have worked too, but by the time I got around to processing this review you were away.
  2. I don't understand the comment about a "recursive design". Do you mean the dictionary of dictionaries? As to whether we need this functionality we may not yet, but it hardly makes the code any more complex and it's quite a lot more general. For example, each CCD could need its own colour term at some point.
  3. I expect the Colorterm.setColorterms() to be in the per-camera config, e.g.
    from lsst.meas.photocal.colorterms import Colorterm
    from lsst.obs.suprimecam.colorterms import colortermsData
    Colorterm.setColorterms(colortermsData)
    
    Colorterm.setActiveDevice("Hamamatsu")
    

I'm going to merge this to master (done; 3e60228), but we can continue to discuss the design behind your second concern.

comment:7 Changed 8 years ago by jbosch

  • Status changed from closed to assigned

These changes broke pipe_tasks, in the same way the unit test was broken.

Fix is on pipe_tasks tickets/2057:

`--> git diff master...tickets/2057
diff --git a/python/lsst/pipe/tasks/calibrate.py b/python/lsst/pipe/tasks/calibrate.py
index 90d3dd6..96cc8b7 100644
--- a/python/lsst/pipe/tasks/calibrate.py
+++ b/python/lsst/pipe/tasks/calibrate.py
@@ -223,7 +223,9 @@ class CalibrateTask(pipeBase.Task):
 
         if self.config.doPhotoCal:
             assert(matches is not None)
-            photocalRet = self.photocal.run(matches)
+            assert(matchMeta is not None)
+            passband = matchMeta.get('FILTER')
+            photocalRet = self.photocal.run(matches, passband)
             zp = photocalRet.photocal
             self.log.log(self.log.INFO, "Photometric zero-point: %f" % zp.getMag(1.0))
             exposure.getCalib().setFluxMag0(zp.getFlux(0))

I'd appreciate a signoff from someone more involved with this ticket before merging in case we actually need to do something more sophisticated.

comment:8 Changed 8 years ago by rhl

  • Status changed from assigned to closed
  • Resolution set to fixed

I failed to push the changes in pipe_tasks. The correct fix is:

diff --git a/python/lsst/pipe/tasks/calibrate.py b/python/lsst/pipe/tasks/calibrate.py
index 9d22464..9860aec 100644
--- a/python/lsst/pipe/tasks/calibrate.py
+++ b/python/lsst/pipe/tasks/calibrate.py
@@ -225,7 +228,7 @@ class CalibrateTask(pipeBase.Task):
 
         if self.config.doPhotoCal:
             assert(matches is not None)
-            photocalRet = self.photocal.run(matches)
+            photocalRet = self.photocal.run(matches, exposure.getFilter().getName())
             zp = photocalRet.photocal
             self.log.log(self.log.INFO, "Photometric zero-point: %f" % zp.getMag(1.0))
             exposure.getCalib().setFluxMag0(zp.getFlux(0))

I pushed this to ncsa/tickets/2057 as 401aec5, and merged to master at c39fe8b

comment:9 in reply to: ↑ 6 Changed 8 years ago by krughoff

Replying to rhl:

In response to number 2, I'm fine with the idea of a dictionary of dictionaries. The thing that initially confused me was that the class potentially contains many copies of itself (the recursive part). This has the added side effect that all the methods in the class need to be static.

Note: See TracTickets for help on using tickets.