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 #48
Conversation
21213d4
to
cde1767
Compare
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.
Looks fine.
@@ -36,6 +36,10 @@ | |||
|
|||
DATA_DIR = os.path.join(os.path.split(__file__)[0], "data") | |||
|
|||
# Note: mosaic's internal ffp object is magnitude-based. | |||
# Instead, for these tests, we just divide this out when comparing. | |||
referenceFlux = 1e23 * 10**(48.6 / -2.5) * 1e9 |
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.
Does this need a #TODO or is this just for the internal tests and meas_mosaic won't be changed even when Calib is retired?
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.
No TODO here: mosaic's internal format will not change.
Update FluxFitBoundedField to produce nanojansky, while leaving its internal persistence format unchanged. The calibrationMean/Err are now nJy though. Note: the existing test files are now incorrect. Will fix on a different commit.
@TallJimbo : do you mind looking at the latest commit on this PR please? I updated the test files and made them work with |
a67cf96
to
80b1975
Compare
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.
Looks good to me. Only comment is almost definitely unrelated to this ticket; just curious.
@@ -142,21 +146,18 @@ def setUp(self): | |||
self.ffp[ccd] = lsst.meas.mosaic.FluxFitParams(fcrMetadata) | |||
wcsFilename = os.path.join( | |||
DATA_DIR, | |||
"%d/wcs-%07d-%03d.fits" % (self.tract, self.visit, ccd) | |||
"%d/jointcal_wcs-%07d-%03d.fits" % (self.tract, self.visit, ccd) |
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.
Presumably not something done on this ticket, but...we renamed "wcs" to "jointcal_wcs"...for meas_mosaic, too? I wasn't paying attention, but I thought the point of the rename must have been to let meas_mosaic and jointcal outputs exist in the same data repo so they could be selected via config (rather than input rerun) in the coaddition tasks.
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.
Yes, it was renamed: see mosaicTask.py:646
.
The goal of the rename was to make it more obvious what the object was. jointcal and meas_mosaic have identical output so that the input to calibration/validation can be swapped between them.
PhotoCalib now needs version 1 (defined in nJy), and I had to re-run meas_mosaic to produce new test files to compare with the ffp outputs. The command I used to generate these was (on lsst-dev, with this branch of utils, afw, and meas_mosaic setup): ``` mosaic.py /datasets/hsc/repo --calib /datasets/hsc/repo/CALIB --rerun DM-13666/WIDE:private/parejkoj/DM-16650 --numCoresForRead=12 --id ccd=0..8^10..103 tract=8766 visit=11442^11446^11450^11470^11476^11478^11506^11508^11532^11534 ```
234b0ca
to
39a227d
Compare
No description provided.