DM-13054: Refactor colorterm calculations to use in jointcal #266
Conversation
parejkoj
force-pushed
the
tickets/DM-13054
branch
3 times, most recently
from
99df351
to
5c131af
Compare
|
|
||
| primaryMag = u.Quantity(primaryFlux, u.Jy).to_value(u.ABmag) | ||
| secondaryMag = u.Quantity(secondaryFlux, u.Jy).to_value(u.ABmag) | ||
|
|
There was a problem hiding this comment.
Is there a ticket for when refcats will be changed to nJy from Jy? (and they are in Jy now, right?)
There was a problem hiding this comment.
DM-17029
And yes, they are in Jy now.
python/lsst/pipe/tasks/colorterms.py
Outdated
| refFluxErrArr = self.propagateFluxErrors(primaryErr, secondaryErr) | ||
|
|
||
| refMagErr = np.array([abMagErrFromFluxErr(rfe, rf) | ||
| for rfe, rf in zip(refFluxErrArr, primaryFlux)]) |
There was a problem hiding this comment.
I just confirmed that abMagErrFromFluxErr works on arrays, so this should be:
refMagErr = abMagErrFromFluxErr(refFluxErrArr, primaryFlux). This is roughly 75x faster and might actually be noticeable for large catalogs.
There was a problem hiding this comment.
Fair point.
That said, the code in this method is already ~20% faster than the code I pulled out of PhotoCalTask.extractMagArrays(), but the actual refcat loading takes so much more time that speeding this method up is almost unnoticeable.
| refCat.reserve(len(matches)) | ||
| for x in matches: | ||
| record = refCat.addNew() | ||
| record.assign(x.first) |
There was a problem hiding this comment.
And we confirmed that this always returns a contiguous catalog? I wasn't sure of the conclusion on slack.
There was a problem hiding this comment.
Yes.
Or, at least, if it didn't, the code just below would fail.
python/lsst/pipe/tasks/photoCal.py
Outdated
| else: | ||
| refMagArr = np.array([abMagFromFlux(rf) for rf in refFluxArrList[0]]) | ||
| refMagArr = u.Quantity(refFluxArr, u.Jy).to_value(u.ABmag) | ||
| refMagErrArr = u.Quantity(refFluxErrArr, u.Jy).to_value(u.ABmag) # WRONG! |
There was a problem hiding this comment.
What is this # WRONG! comment doing here?
There was a problem hiding this comment.
Because I wrote something that I knew was wrong and had meant to fix it and forgot. Thanks for catching it.
| refMag, refMagErr = self.colorterm.getCorrectedMagnitudes(refCat, 'r') | ||
| self.assertEqual(refMag, expectMag) | ||
| # NOTE: Not testing the returned errors, as I do not trust `propagateFluxErrors()` | ||
| # and there is some interesting logic involved in how the errors are propagated. |
There was a problem hiding this comment.
I think this needs to be ticketed. And I (at least) care about the reference flux error propagation.
There was a problem hiding this comment.
python/lsst/pipe/tasks/photoCal.py
Outdated
| @@ -386,8 +374,6 @@ def extractMagArrays(self, matches, filterName, sourceKeys): | |||
|
|
|||
| srcMagErrArr = np.array([abMagErrFromFluxErr(sfe, sf) | |||
| for sfe, sf in zip(srcInstFluxErrArr, srcInstFluxArr)]) | |||
There was a problem hiding this comment.
This should also be vectorized as above. There are probably others in the code that weren't put into the GitHub change-log, which maybe outside the scope of this ticket, but shouldn't be too hard to do while doing the rearranging.
parejkoj
force-pushed
the
tickets/DM-13054
branch
from
96007f7
to
1950f1f
Compare
The existing code made it very painful to compute the colorterm corrections. Now `Colorterm.getCorrectedMagnitudes()` does all the necessary work. I am skeptical of the magnitude error calculations here, but I'm just copying them from PhotoCalTask, so they're at least not any more wrong than before. This change does fix a serious bug in the refMagErr calculation: it was previously being done using the secondary flux, not the primary flux (the loop was overwriting `refFluxArr` before it was used to calculate the refMagErr).
parejkoj
force-pushed
the
tickets/DM-13054
branch
from
1950f1f
to
c54f7bb
Compare
No description provided.