Tidy up variable names, docstrings, and error handling

python/lsst/ap/pipe/ap_pipe.py

@@ -33,7 +33,7 @@
 
 from __future__ import absolute_import, division, print_function
 
-__all__ = ['get_datafiles', 'get_calibdatafiles', 'get_defectfiles', 'get_output_repos',
+__all__ = ['get_datafiles', 'get_calib_datafiles', 'get_defectfiles', 'get_output_repo',
            'doIngest', 'doIngestCalibs', 'doProcessCcd', 'doDiffIm',
            'runPipelineAlone']
 
@@ -59,9 +59,6 @@
 from lsst.pipe.tasks.imageDifference import ImageDifferenceConfig, ImageDifferenceTask
 import lsst.daf.persistence as dafPersist
 
-# IN PROGRESS: figure out which of these ALL-CAPS VARIABLES are already known by
-# ap_verify and which need to be explicitly provided in a function here in ap_pipe
-
 # Names of directories containing data products in dataset_root
 RAW_DIR = 'raw'
 MASTERCAL_DIR = 'calib'
@@ -100,7 +97,7 @@ def get_datafiles(raw_location):
     return datafiles
 
 
-def get_calibdatafiles(calib_location):
+def get_calib_datafiles(calib_location):
     '''
     Retrieve a list of the DECam MasterCal flat and bias files for use during ingestion.
 
@@ -111,31 +108,31 @@ def get_calibdatafiles(calib_location):
 
     Returns
     -------
-    calibdatafiles: `list`
+    calib_datafiles: `list`
         A list of the filenames of each flat and bias image file.
     '''
     types = ('*.fits', '*.fz')
-    allcalibdatafiles = []
+    all_calib_datafiles = []
     for files in types:
-        allcalibdatafiles.extend(glob(os.path.join(calib_location, files)))
+        all_calib_datafiles.extend(glob(os.path.join(calib_location, files)))
     # Ignore wtmaps and illumcors
-    calibdatafiles = []
-    filestoignore = ['fcw', 'zcw', 'ici']
-    for file in allcalibdatafiles:
-        if all(string not in file for string in filestoignore):
-            calibdatafiles.append(file)
-    return calibdatafiles
+    calib_datafiles = []
+    files_to_ignore = ['fcw', 'zcw', 'ici']
+    for file in all_calib_datafiles:
+        if all(string not in file for string in files_to_ignore):
+            calib_datafiles.append(file)
+    return calib_datafiles
 
 
-def get_defectfiles(defect_location, DEFECT_TARBALL=DEFECT_TARBALL):
+def get_defectfiles(defect_location, defect_tarball=DEFECT_TARBALL):
     '''
     Retrieve a list of the DECam defect files for use during ingestion.
 
     Parameters
     ----------
     defect_location: `str`
         The path on disk to where the defect tarball lives.
-    DEFECT_TARBALL: `str`
+    defect_tarball: `str`
         The filename of the tarball containing the defect files.
 
     Returns
@@ -146,32 +143,32 @@ def get_defectfiles(defect_location, DEFECT_TARBALL=DEFECT_TARBALL):
         which contains all the compressed defect images.
     '''
     # Retrieve defect filenames from tarball
-    defect_tarfile_path = glob(os.path.join(defect_location, DEFECT_TARBALL))[0]
+    defect_tarfile_path = os.path.join(defect_location, defect_tarball)
     defectfiles = tarfile.open(defect_tarfile_path).getnames()
     defectfiles = [os.path.join(defect_location, file) for file in defectfiles]
     return defectfiles
 
 
-def get_output_repos(output_root, output_dir):
+def get_output_repo(output_root, output_dir):
     '''
-    Define locations on disk for all of the output repositories used by ap_pipe.
+    Return location on disk for one output repository used by ap_pipe.
 
     Parameters
     ----------
     output_root: `str`
         The top-level directory where the output will live.
     output_dir: `str`
-        Name of directory to be created in output_root.
+        Name of the subdirectory to be created in output_root.
 
     Returns
     -------
-    result: `str`
+    output_path: `str`
         Repository (directory on disk) where desired output product will live.
     '''
     if not os.path.isdir(output_root):
         os.mkdir(output_root)
-    result = os.path.join(output_root, output_dir)
-    return result
+    output_path = os.path.join(output_root, output_dir)
+    return output_path
 
 
 def parsePipelineArgs():
@@ -183,15 +180,14 @@ def parsePipelineArgs():
     repos_and_files: `dict`
         Includes the names of new repos that will be written to disk
         following ingestion, calib ingestion, processing, and difference imaging
-        (repo, calib_repo, processed_repo, diffim_repo)
+        ('repo', 'calib_repo', 'processed_repo', 'diffim_repo')
         Includes the files in dataset_root for raw images, flats and biases,
-        and defects (datafiles, calibdatafiles, defectfiles)
+        and defects ('datafiles', 'calib_datafiles', 'defectfiles')
+        Finally includes the path on disk of the reference catalogs ('refcats')
     idlist: `list` containing two `str`
         Data ID and template info needed for processing and difference imaging
         [dataId, template]
         TODO: allow 'template' to be either a visit ID or a repo name (DM-11422)
-    refcats: `str`
-        Path on disk of the reference catalogs
     '''
 
     # Parse command line arguments with argparse
@@ -214,14 +210,14 @@ def parsePipelineArgs():
 
     # Retrieve lists of input files for raw images and calibration products
     datafiles = get_datafiles(os.path.join(args.dataset_root, RAW_DIR))
-    calibdatafiles = get_calibdatafiles(os.path.join(args.dataset_root, MASTERCAL_DIR))
+    calib_datafiles = get_calib_datafiles(os.path.join(args.dataset_root, MASTERCAL_DIR))
     defectfiles = get_defectfiles(os.path.join(args.dataset_root, DEFECT_DIR))
 
     # Define output repo locations on disk
-    repo = get_output_repos(args.output, INGESTED_DIR)
-    calib_repo = get_output_repos(args.output, CALIBINGESTED_DIR)
-    processed_repo = get_output_repos(args.output, PROCESSED_DIR)
-    diffim_repo = get_output_repos(args.output, DIFFIM_DIR)
+    repo = get_output_repo(args.output, INGESTED_DIR)
+    calib_repo = get_output_repo(args.output, CALIBINGESTED_DIR)
+    processed_repo = get_output_repo(args.output, PROCESSED_DIR)
+    diffim_repo = get_output_repo(args.output, DIFFIM_DIR)
 
     # Retrieve location of refcats directory containing gaia and pan-starrs tarballs
     refcats = os.path.join(args.dataset_root, REFCATS_DIR)
@@ -235,10 +231,11 @@ def parsePipelineArgs():
     repos_and_files = {'repo': repo, 'calib_repo': calib_repo,
                        'processed_repo': processed_repo,
                        'diffim_repo': diffim_repo, 'datafiles': datafiles,
-                       'calibdatafiles': calibdatafiles, 'defectfiles': defectfiles}
+                       'calib_datafiles': calib_datafiles, 'defectfiles': defectfiles,
+                       'refcats': refcats}
     idlist = [args.dataId, template]
 
-    return repos_and_files, idlist, refcats
+    return repos_and_files, idlist
 
 
 def doIngest(repo, refcats, datafiles):
@@ -280,7 +277,6 @@ def doIngest(repo, refcats, datafiles):
     ASTROM_REFCAT_DIR = 'ref_cats/gaia'
     PHOTOM_REFCAT_DIR = 'ref_cats/pan-starrs'
 
-    lsst.log.configure()
     log = lsst.log.Log.getLogger('ap.pipe.doIngest')
     if os.path.exists(os.path.join(repo, 'registry.sqlite3')):
         log.warn('Raw images were previously ingested, skipping...')
@@ -318,7 +314,7 @@ def doIngest(repo, refcats, datafiles):
     return ingest_metadata
 
 
-def flatBiasIngest(repo, calib_repo, calibdatafiles):
+def flatBiasIngest(repo, calib_repo, calib_datafiles):
     '''
     Ingest DECam flats and biases (called by doIngestCalibs)
 
@@ -328,7 +324,7 @@ def flatBiasIngest(repo, calib_repo, calibdatafiles):
         The output repository location on disk where ingested raw images live.
     calib_repo: `str`
         The output repository location on disk where ingested calibration images live.
-    calibdatafiles: `list`
+    calib_datafiles: `list`
         A list of the filenames of each flat and bias image file.
 
     Returns
@@ -337,13 +333,12 @@ def flatBiasIngest(repo, calib_repo, calibdatafiles):
         Metadata from the IngestCalibTask (flats and biases) for use by ap_verify
 
     BASH EQUIVALENT:
-    $ ingestCalibs.py repo --calib calib_repo --mode=link --validity 999 calibdatafiles
+    $ ingestCalibs.py repo --calib calib_repo --mode=link --validity 999 calib_datafiles
     '''
-    lsst.log.configure()
     log = lsst.log.Log.getLogger('ap.pipe.flatBiasIngest')
     log.info('Ingesting flats and biases...')
     args = [repo, '--calib', calib_repo, '--mode', 'link', '--validity', '999']
-    args.extend(calibdatafiles)
+    args.extend(calib_datafiles)
     argumentParser = IngestCalibsArgumentParser(name='ingestCalibs')
     config = IngestCalibsConfig()
     config.parse.retarget(ingestCalibs.DecamCalibsParseTask)
@@ -354,7 +349,7 @@ def flatBiasIngest(repo, calib_repo, calibdatafiles):
     except sqlite3.IntegrityError as detail:
         log.error('sqlite3.IntegrityError: ', detail)
         log.error('(sqlite3 doesn\'t think all the calibration files are unique)')
-        flatBias_metadata = None
+        raise
     else:
         log.info('Success!')
         log.info('Calibrations corresponding to {0} are now ingested in {1}'.format(repo, calib_repo))
@@ -393,7 +388,6 @@ def defectIngest(repo, calib_repo, defectfiles):
     - They will be manually uncompressed and saved in calib_repo/defects/<tarballname>/.
     - They will be added to the calib registry, but not linked like the flats and biases
     '''
-    lsst.log.configure()
     log = lsst.log.Log.getLogger('ap.pipe.defectIngest')
     os.chdir(calib_repo)
     try:
@@ -402,9 +396,10 @@ def defectIngest(repo, calib_repo, defectfiles):
         # most likely the defects directory already exists
         if os.path.isdir('defects'):
             log.warn('Defects were previously ingested, skipping...')
+            defect_metadata = None
         else:
             log.error('Defect ingestion failed because \'defects\' dir could not be created')
-        defect_metadata = None
+            raise
     else:
         log.info('Ingesting defects...')
         defectargs = ['../../' + repo, '--calib', '.', '--calibType', 'defect',
@@ -425,7 +420,7 @@ def defectIngest(repo, calib_repo, defectfiles):
     return defect_metadata
 
 
-def doIngestCalibs(repo, calib_repo, calibdatafiles, defectfiles):
+def doIngestCalibs(repo, calib_repo, calib_datafiles, defectfiles):
     '''
     Ingest DECam MasterCal biases and flats into a calibration repository with a corresponding registry.
     Also ingest DECam defects into the calib registry.
@@ -436,7 +431,7 @@ def doIngestCalibs(repo, calib_repo, calibdatafiles, defectfiles):
         The output repository location on disk where ingested raw images live.
     calib_repo: `str`
         The output repository location on disk where ingested calibration images live.
-    calibdatafiles: `list`
+    calib_datafiles: `list`
         A list of the filenames of each flat and bias image file.
     defectfiles: `list`
             A list of the filenames of each defect image file.
@@ -450,7 +445,7 @@ def doIngestCalibs(repo, calib_repo, calibdatafiles, defectfiles):
         IngestCalibTask (defects) for use by ap_verify
 
     RESULT:
-    calib_repo populated with *links* to calibdatafiles,
+    calib_repo populated with *links* to calib_datafiles,
     organized by date (bias and flat images only)
     sqlite3 database registry of ingested calibration products (bias, flat,
     and defect images) created in calib_repo
@@ -459,18 +454,17 @@ def doIngestCalibs(repo, calib_repo, calibdatafiles, defectfiles):
     calib ingestion ingests *all* the calibs, not just the ones needed
     for certain visits. We may want to ...revisit... this in the future.
     '''
-    lsst.log.configure()
     log = lsst.log.Log.getLogger('ap.pipe.doIngestCalibs')
     if not os.path.isdir(calib_repo):
         os.mkdir(calib_repo)
-        flatBias_metadata = flatBiasIngest(repo, calib_repo, calibdatafiles)
+        flatBias_metadata = flatBiasIngest(repo, calib_repo, calib_datafiles)
         defect_metadata = defectIngest(repo, calib_repo, defectfiles)
     elif os.path.exists(os.path.join(calib_repo, 'cpBIAS')):
         log.warn('Flats and biases were previously ingested, skipping...')
         flatBias_metadata = None
         defect_metadata = defectIngest(repo, calib_repo, defectfiles)
     else:
-        flatBias_metadata = flatBiasIngest(repo, calib_repo, calibdatafiles)
+        flatBias_metadata = flatBiasIngest(repo, calib_repo, calib_datafiles)
         defect_metadata = defectIngest(repo, calib_repo, defectfiles)
     # Handle the case where one or both of the calib metadatas may be None
     if flatBias_metadata is not None:
@@ -519,7 +513,6 @@ def doProcessCcd(repo, calib_repo, processed_repo, dataId):
     By default, the configuration for astrometric reference catalogs uses Gaia
     and the configuration for photometry reference catalogs uses Pan-STARRS.
     '''
-    lsst.log.configure()
     log = lsst.log.Log.getLogger('ap.pipe.doProcessCcd')
     dataId_items = re.split('[ +=]', dataId)
     dataId_dict = dict(zip(dataId_items[::2], dataId_items[1::2]))
@@ -538,8 +531,8 @@ def doProcessCcd(repo, calib_repo, processed_repo, dataId):
     butler = dafPersist.Butler(inputs={'root': repo, 'mapperArgs': {'calibRoot': calib_repo}},
                                outputs=processed_repo)
     config = ProcessCcdConfig()
-    config.load(OBS_DECAM_DIR + '/config/processCcd.py')
-    config.load(OBS_DECAM_DIR + '/config/processCcdCpIsr.py')
+    config.load(os.path.join(OBS_DECAM_DIR, 'config/processCcd.py'))
+    config.load(os.path.join(OBS_DECAM_DIR, 'config/processCcdCpIsr.py'))
     config.calibrate.doAstrometry = True
     config.calibrate.doPhotoCal = True
     # Use gaia for astrometry (phot_g_mean_mag is only available DR1 filter)
@@ -604,13 +597,11 @@ def doDiffIm(processed_repo, dataId, template, diffim_repo):
         config.doDecorrelation = True
 
     TODO: use coadds as templates by default, not another visit (DM-11422).
-    Some of the comments in this function are placeholders for DM-11422 work.
 
     RESULT:
     diffim_repo/deepDiff/v+visit populated with difference images
     and catalogs of detected sources (diaSrc, diffexp, and metadata files)
     '''
-    lsst.log.configure()
     log = lsst.log.Log.getLogger('ap.pipe.doDiffIm')
     dataId_items = re.split('[ +=]', dataId)
     dataId_dict = dict(zip(dataId_items[::2], dataId_items[1::2]))
@@ -649,25 +640,27 @@ def runPipelineAlone():
     '''
     lsst.log.configure()
     log = lsst.log.Log.getLogger('ap.pipe.runPipelineAlone')
-    repos_and_files, idlist, refcats = parsePipelineArgs()
+    repos_and_files, idlist = parsePipelineArgs()
 
     repo = repos_and_files['repo']
     calib_repo = repos_and_files['calib_repo']
     processed_repo = repos_and_files['processed_repo']
     diffim_repo = repos_and_files['diffim_repo']
 
     datafiles = repos_and_files['datafiles']
-    calibdatafiles = repos_and_files['calibdatafiles']
+    calib_datafiles = repos_and_files['calib_datafiles']
     defectfiles = repos_and_files['defectfiles']
 
+    refcats = repos_and_files['refcats']
+
     dataId = idlist[0]
     template = idlist[1]
 
     dataId_template = 'visit=410929 ccdnum=25'  # temporary
 
     # Run all the tasks in order
     doIngest(repo, refcats, datafiles)
-    doIngestCalibs(repo, calib_repo, calibdatafiles, defectfiles)
+    doIngestCalibs(repo, calib_repo, calib_datafiles, defectfiles)
     doProcessCcd(repo, calib_repo, processed_repo, dataId)
     doProcessCcd(repo, calib_repo, processed_repo, dataId_template)  # temporary
     doDiffIm(processed_repo, dataId, template, diffim_repo)