New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-16650: update to new nJy PhotoCalib definition #120
Conversation
@@ -142,7 +141,7 @@ def compute(catalogs, photoCalibs): | |||
for ref in data_refs: | |||
calib = ref.get('calexp_calib') | |||
fluxMag0 = calib.getFluxMag0() | |||
old_calibs.append(lsst.afw.image.PhotoCalib(1.0/fluxMag0[0], fluxMag0[1]/fluxMag0[0]**2)) | |||
old_calibs.append(lsst.afw.image.PhotoCalib(1e9/fluxMag0[0], 1e9*fluxMag0[1]/fluxMag0[0]**2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have a #TODO associated with it?
|
||
# NOTE: Have to protect against negative reference fluxes too. | ||
# NOTE: Have to protect against negative "reference" fluxes too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off-topic, but can you use a sourceSelector on the reference catalog for a s/n or flux>0 cut?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the "reference" catalog is actually just another source catalog being used as the "reference" values. Which could also be SourceSelected on. But see below...
ref_flux = match[0][ref_flux_key.format(filt)] | ||
# refcat fluxes are already in Janskys. | ||
# TODO: Once RFC-549 is fully implemented, we can remove the 1e9 prefactor. | ||
ref_flux = 1e9 * match[0][ref_flux_key.format(filt)] | ||
if ref_flux < 0: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a bit off-topic, and I don't know how long jointcal spends doing this collation, but I think this whole section can be numpy-array-ized and would avoid a lot of logic and loops. In the end the return value of _make_match_dict
is numpy arrays anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I'm not at all happy about the code here in utils.py
. I wrote it when I was trying to come up with a way to verify that joincal was doing better than processCcd, and it ended up a big mess over time. It is used in jointcal's test suite, but not elsewhere and the plots it can make have helped me debug things in the past.
However, you are correct, the calculations done in this file are a non-trivial part of the time running jointcal's tests due to the crappy way they were implemented. Ideally, I'd throw almost all of this away and just call the AM1, PA1 calculations from validate_drp. That is sort of DM-6577, but not exactly: I can't make validate_drp a dependency of jointcal, and the question of where to put the metric calculation code has repeatedly stalled.
There's talk of doing a hack week at Princeton to develop some better metrics and analysis of jointcal's output: we might be able to replace all of this then.
Long story short: I completely agree, but it's too much work for this ticket.
Use new utils Magnitude header for nJy/mag conversions in C++, and astropy.units in python. This adds a number of unfortunate hacks related to our refcats still being in Jy (not nJy), but they are all called out with reference to RFC-549. Moved the PhotometryModel validate() and checkPositiveOnBBox() tests into the flux model, because it's a lot easier to make a flux-based model go negative.
aafaa21
to
84df437
Compare
No description provided.