DM-25424: Convert Defect to IsrCalib #164
Conversation
|
@czwa I've pushed a couple of commits to switch to GitHub Actions because Travis had hung up again. To merge I need to turn off the travis requirement -- I can do that any time but better to do it when you are about to merge. |
There was a problem hiding this comment.
Looks good. Minor comments since this is mostly copying stuff around.
Since this is mostly copying my code from another package, can you please reorganize the commits so that you have one commit that is copying the files from meas_algorithms (defects.py and the test code) and then when you commit it make use of the co-author feature so that I turn up somewhere if people start doing a git blame? See eg https://docs.github.com/en/free-pro-team@latest/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors
python/lsst/ip/isr/calibType.py
Outdated
| @@ -209,7 +215,7 @@ def updateMetadata(self, camera=None, detector=None, filterName=None, | |||
| self._metadata["INSTRUME"] = self._instrument if self._instrument else None | |||
| self._metadata["RAFTNAME"] = self._raftName if self._raftName else None | |||
| self._metadata["SLOTNAME"] = self._slotName if self._slotName else None | |||
| self._metadata["DETECTOR"] = self._detectorId if self._detectorId else None | |||
| self._metadata["DETECTOR"] = self._detectorId if self._detectorId is not None else None | |||
There was a problem hiding this comment.
I don't understand what this is doing. As far I can tell it is saying to set this entry to the value in the object if it it not None but to set it to None if it is None. Isn't that the same as letting it set the value directly? Since the only case where you get None is if the self._detectorId was itself None and therefore you get the extra logic without any gain.
The other lines are converting False to None but this one is converting None to None.
There was a problem hiding this comment.
This was a fix for the case where self._detectorId = 0. _detectorId is now expected to be an integer, not a string, and in this case if self._detectorId returns false, converting the integer 0 into a None. In turn, this was breaking round-trip tests.
There was a problem hiding this comment.
I still don't understand though. I must be missing something.
For:
x = self._detectorId if self._detectorId is not None else NoneThis can be rewritten as:
if self._detectorId is not None:
x = self_detectorId
else:
x = Nonewhich is the same as:
if self._detectorId is None:
x = None
else:
x = self._detectorIdwhich is the same as:
x = self._detectorIdisn't it?
There was a problem hiding this comment.
I agree with all that, leading me to believe the test case failure confused me. Replaced with the simple version.
There was a problem hiding this comment.
I'm sure you're are right that self._detectorId if self._detectorId else None was wrong because of the 0 so this did need fixing.
| @@ -273,7 +279,7 @@ def search(haystack, needles): | |||
| self._calibId = search(dictionary, ['CALIB_ID']) | |||
|
|
|||
| @classmethod | |||
| def readText(cls, filename): | |||
| def readText(cls, filename, **kwargs): | |||
There was a problem hiding this comment.
You need to document the purpose of these kwargs arguments.
python/lsst/ip/isr/defects.py
Outdated
| # | ||
| # LSST Data Management System | ||
| # | ||
| # Copyright 2008-2017 AURA/LSST. |
| @@ -0,0 +1 @@ | |||
| SIMPLE = T / conforms to FITS standard BITPIX = 8 / array data type NAXIS = 0 / number of array dimensions EXTEND = T END XTENSION= 'BINTABLE' / binary table extension BITPIX = 8 / array data type NAXIS = 2 / number of array dimensions NAXIS1 = 119 / length of dimension 1 NAXIS2 = 9 / length of dimension 2 PCOUNT = 0 / number of group parameters GCOUNT = 1 / number of groups TFIELDS = 6 / number of table fields EXTNAME = 'REGION ' EXTVER = 1 EXTLEVEL= 1 HDUNAME = 'REGION ' HDUCLASS= 'ASC ' HDUCLAS1= 'REGION ' HDUCLAS2= 'STANDARD' HDUVERS = '1.2.0 ' HDUDOC = 'ASC-FITS-REGION-1.2: Rots, McDowell: FITS REGION Binary Table Design'CONTENT = 'REGION ' TELESCOP= 'CHANDRA ' DATAMODE= 'FAINT ' INSTRUME= 'ACIS ' OBJECT = 'M101 ' ONTIME = 99514.55727793276 DEADC = 0.98733741 DEADAPP = F EXPOSURE= 98254.44528487051 LIVETIME= 98254.44528487051 DATE-OBS= '2000-03-26T00:20:55' DATE-END= '2000-03-27T04:40:34' TSTART = 70418401.45661506 TSTOP = 70518359.85670815 TELAPSE = 99958.40009309351 MJD-OBS = 51629.02779463675 MJDREF = 50814.0 TIMEREF = 'LOCAL ' TIMESYS = 'TT ' TIMEUNIT= 's ' EQUINOX = 2000.0 RADECSYS= 'ICRS ' USER = 'pence ' FILIN001= 'acisf00934N002_evt2.fits' CREATOR = 'extractor v4.84' DATE = '2008-06-20T16:49:07' ORIGIN = 'NASA/GSFC' MTYPE1 = 'sky ' MFORM1 = 'x,y ' MTYPE2 = 'EQPOS ' MFORM2 = 'RA,DEC ' TCTYP1 = 'RA---TAN' TCRPX1 = 4096.5 TDRPX1 = 4096.5 TCRVL1 = 210.7801525309 TCDLT1 = -0.00013666666666667 TDDLT1 = -0.00013666666666667 TCTYP2 = 'DEC--TAN' TCRPX2 = 4096.5 TDRPX2 = 4096.5 TCRVL2 = 54.366791304488 TCDLT2 = 0.00013666666666667 TDDLT2 = 0.00013666666666667 TCROT2 = 0.0 CHECKSUM= 'ZmaGdkTDZkYDdkYD' DATASUM = '3203022982' TTYPE1 = 'X ' TFORM1 = '4D ' TUNIT1 = 'pix ' TTYPE2 = 'Y ' TFORM2 = '4D ' TUNIT2 = 'pix ' TTYPE3 = 'SHAPE ' TFORM3 = '7A ' TTYPE4 = 'R ' TFORM4 = '4D ' TUNIT4 = 'pix ' TTYPE5 = 'ROTANG ' TFORM5 = 'D ' TUNIT5 = 'deg ' TTYPE6 = 'COMPONENT' TFORM6 = 'K ' END @�� @�� circle @wߋ�q � @���b� @����e� ROTBOX @�]ֻ���@�x��!� @<e*b�D @� @�! ellipse@r @� @uz��� @uP @u� ROTBOX @L @S� @P@ @uP @u� ANNULUS@L @S� @uP @u� point @uP @u� point ?� @ @ @ @ @ @ @ polygon @$ @ BOX @$ @4 | |||
There was a problem hiding this comment.
You should add a .gitattributes file to declare this as a binary. Copy the one from meas_algorithms.
There was a problem hiding this comment.
can you reorganize the commit history so that the .gitattributes file turns up before the fits file is committed?
czwa
force-pushed
the
tickets/DM-25424
branch
from
a005290
to
ada5441
Compare
I did not know that was an option, but my rebased commit: b575777 seems to have the attribution correct now. |
tickets/DM-25424-transfer. The C++ Defect type is staying in meas_algorithms, as the interpolation and cosmic ray code there depend on it. Co-authored-by: Tim Jenness <tjenness@lsst.org>
This resolves build issues that were introduced by transferring the Defects code from meas_algorithms.
This implements the pass-through of normalize_on_init to the read methods, so obs_ package data can be read faster (as it is assumed to be pre-normalized).
* Convert the fits.card.Undefined values back to python None. * Use a common calibInfoFromDict method to set calibration properties from the input dictionary/metadata.
czwa
force-pushed
the
tickets/DM-25424
branch
from
ada5441
to
623ded7
Compare
Migrate Defects to ip_isr and make an IsrCalib.