DM-11021: Generalize prototype pipeline script #4

mrawls · 2017-07-28T23:28:54Z

No description provided.

Defect ingestion does not work presently because the defects live in a tar.gz file. Once this is fixed, the pipeline should work. Additional outstanding issues are noted with TODO in the code.

Gaia for astrometry, pan-starrs for photometry These changes effectively complete DM-10975

kfindeisen · 2017-07-31T23:29:56Z

decam_process.py

+        datafiles.extend(glob(os.path.join(args.dataset, 'raw', files)))
+        allcalibdatafiles.extend(glob(os.path.join(args.dataset, 'calib', files)))
+
+    # Ignore wtmaps and illumcors


You might consider breaking up main() into several functions, e.g., one for argument parsing, one for finding the files, one for the "run everything" bit... Stuff could be divided past that; for example, the code used to create datafiles and allcalibdatafiles could be split into a single function that takes a directory and an extension (or sequence thereof).

I wholeheartedly agree.

Fair enough! Mucking with main() is a bit lower on my priority list since ap_verify doesn't care about it, but you're both correct that this function is rather lengthy with multiple functionalities. I'll see what I can do.

djreiss

Overall looks like a very positive improvement.

djreiss · 2017-08-04T21:24:22Z

decam_process.py

@@ -15,324 +15,378 @@
 import os
 import argparse
 import textwrap
+import tarfile


Std lib imports should come before local lib imports: https://developer.lsst.io/v/DM-5063/coding/python_style_guide.html#imports-should-be-grouped-in-order-standard-lib-3rd-party-lib-local-lib

djreiss · 2017-08-04T21:25:33Z

decam_process.py

 '''
 Process raw decam images with MasterCals from ingestion --> difference imaging

+TODO: Rename script and GitHub repo to ap_pipe (not decam_hits or decam_process).


Module-level docstring should come above the imports.

djreiss · 2017-08-04T21:41:40Z

decam_process.py

-$ python hits_ingest.py ingestCalibs -f path/to/calibrations/somefiles*.fits.fz
-$ python hits_ingest.py processCcd
-$ python hits_ingest.py diffIm
+$ python decam_process.py -d dataset_root -o output_location -i "visit=12345, ccd=5"


Will need to make sure that this line stays sync-ed with the one on L49 - or else get rid of it here...?

djreiss · 2017-08-04T21:42:30Z

decam_process.py

+                        help="Location on disk of dataset_root, which contains subdirectories of raw data, calibs, etc.")
+    parser.add_argument('-o', '--output',
+                        help="Location on disk where output repos will live.")
+    # TODO: implement this argument


Is there a ticket for this?

It is part of DM-11422 because using coadds as templates will require --id-like metadata to be handled differently than when a visit is a template. I'll note this explicitly here.

For future reference: https://developer.lsst.io/coding/python_style_guide.html#to-do-comments-should-include-a-jira-issue-key 🙂

djreiss · 2017-08-04T21:43:18Z

decam_process.py

-    ingest - Ingest raw DECam images into a repository with a
-    corresponding registry.
-    USAGE:
-    $ python hits_ingest.py ingest -f path/to/rawimages/


Did all of this extra functionality go away?

Essentially yes; a call to run the pipeline script via main() will run all of the steps rather than just one at a time. This doesn't matter for ap_verify (or any other external code using the pipeline) because it can call whichever function it wants whenever it wants. Is that OK?

djreiss · 2017-08-04T22:27:38Z

decam_process.py

+            tarfile.open(os.path.join('../../', defecttarball), 'r').extractall('defects')
+            defectfiles = glob(os.path.join('defects', os.path.basename(defectfiles[0]), '*.fits'))
+            defectargs.extend(defectfiles)
+            defectargumentParser = IngestCalibsArgumentParser(name='ingestCalibs')


defectArgumentParser, etc...

djreiss · 2017-08-04T22:28:32Z

decam_process.py

-        print('If this is your first time ingesting these calibration files,')
-        print('  make sure you only use image or wtmap MasterCals, not both.')
-        print('~~~ !!! ~~~')
+        flatBiasIngest(repo, calibrepo, calibdatafiles)


Maybe not for this ticket but I think all of this code would benefit from going into a class, instead of having passing around all the different variables from one function to the next.

Can you be more specific about the benefits? You made a similar argument for ap_verify and I'm still not sure it was the right thing to do; in particular, explicit function arguments make it much easier to see (and control) each task's data dependencies.

Happy to discuss with both of you further, but "not for this ticket" indeed!

djreiss · 2017-08-04T22:30:02Z

decam_process.py

-        refObjLoader.filterMap = {"g": "g",
-                                  "r": "r",
-                                  "VR": "g"}
+    ** to run from bash, 'processccd_config.py' must exist and contain


What is the equivalent of using the processccd_config.py file for this script? Is it hardcoded somewhere, or not used?

Nevermind, I see it below. Might be good to enumerate the default values that are set in the config (e.g. using gaia for astrometric reference, etc.)

djreiss · 2017-08-04T22:32:15Z

decam_process.py

-            'calibrate.doPhotoCal=True']
-    ProcessCcdTask.parseAndRun(args=args, config=config)
+    if os.path.isdir(os.path.join(processedrepo, '0'+visit)):
+        print('ProcessCcd has already been run for visit {0}, skipping...'.format(visit))


Again, maybe add a return statement here and move everything else out of the else block.

djreiss · 2017-08-04T22:36:25Z

decam_process.py

+
+    Parameters
+    ----------
+    processedrepo: `str`


I first read this as proccess drepo. I like camel-case or underscore (I don't care which but I think the style guide has us moving toward using underscore, e.g. processed_repo.

mrawls · 2017-08-10T20:19:58Z

Thanks for the thorough review! I think the only outstanding items are (1) adding tests and (2) possibly changing how defect file/tarball info is passed into doIngestCalibs. I would like to close this ticket so I can rename/reorganize the repo to finally be ap_pipe before making many more changes to the code. I propose handling these outstanding items as part of DM-11390 if that is OK with you.

djreiss

All looks great. Just 2 minor comments.

djreiss · 2017-08-10T21:51:19Z

decam_process.py

@@ -207,158 +246,307 @@ def doIngest(repo, datafiles):
    config = IngestConfig()
    config.parse.retarget(DecamParseTask)
    # create an instance of the decam ingest task
-    ingestTask = ingest.DecamIngestTask(config=config)
+    IngestTask = ingest.DecamIngestTask(config=config)


Is there a reason you changed this to capitalized? Capitalized names are for classes, whereas this is a class instance?

Probably because the story of my life is being confused about what is a class and what is an instance... I'll change it back :)

djreiss · 2017-08-10T21:51:56Z

decam_process.py

    argumentParser = IngestCalibsArgumentParser(name='ingestCalibs')
    config = IngestCalibsConfig()
    config.parse.retarget(ingestCalibs.DecamCalibsParseTask)
-    ingestTask = IngestCalibsTask(config=config, name='ingestCalibs')
+    CalibIngestTask = IngestCalibsTask(config=config, name='ingestCalibs')


Same comment as above.

djreiss · 2017-08-10T21:53:17Z

decam_process.py

+    os.chdir(calib_repo)
+    try:
+        os.mkdir('defects')
+    except OSError:


I like the change over to try/except/else!

Never mind, it's always OSError. 😲

You'll probably brand this as "not this ticket", but I don't think it's safe to assume a pre-existing directory is the only reason os.mkdir raises -- it could also be lack of permissions, problems with the filesystem itself, etc...

Fair point @kfindeisen, I will make the statement that prints with that error mention the fact that the exception is likely (but not necessarily) happening because the defect directory already exists.

The defects are now copied and extracted from the input dataset into the calibrepo, much in the same way ref_cats are extracted into repo. This is done during ingestion instead of during processing.

mrawls added 4 commits July 18, 2017 13:22

Update README to point to DMTN-039.

7157e4c

Ingest defects as part of doIngestCalibs and update docstrings

95f24c6

Refactor decam_process to use verify_ap_hits2015 dataset.

282b410

Defect ingestion does not work presently because the defects live in a tar.gz file. Once this is fixed, the pipeline should work. Additional outstanding issues are noted with TODO in the code.

Use ref_cats from ap_verify_hits2015 dataset

b7b5195

Gaia for astrometry, pan-starrs for photometry These changes effectively complete DM-10975

kfindeisen reviewed Jul 31, 2017

View reviewed changes

djreiss reviewed Aug 4, 2017

View reviewed changes

djreiss reviewed Aug 10, 2017

View reviewed changes

mrawls added 3 commits August 10, 2017 18:42

Add control flow, fully retrieve defect files, update comments

9b33a7b

The defects are now copied and extracted from the input dataset into the calibrepo, much in the same way ref_cats are extracted into repo. This is done during ingestion instead of during processing.

Split main into two functions, reorganize imports, fewer hardwired paths

e46f05c

Each function now returns metadata, plus variable renaming for clarity

bc8f42c

mrawls force-pushed the tickets/DM-11021 branch from b0514c1 to bc8f42c Compare August 11, 2017 01:43

mrawls merged commit bc8f42c into master Aug 11, 2017

kfindeisen deleted the tickets/DM-11021 branch April 13, 2022 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-11021: Generalize prototype pipeline script #4

DM-11021: Generalize prototype pipeline script #4

mrawls commented Jul 28, 2017

kfindeisen Jul 31, 2017 •

edited

djreiss Aug 4, 2017

mrawls Aug 5, 2017

djreiss left a comment

djreiss Aug 4, 2017

djreiss Aug 4, 2017

djreiss Aug 4, 2017

djreiss Aug 4, 2017

mrawls Aug 5, 2017

kfindeisen Aug 7, 2017 •

edited

djreiss Aug 4, 2017

mrawls Aug 5, 2017

djreiss Aug 4, 2017

djreiss Aug 4, 2017

kfindeisen Aug 7, 2017

mrawls Aug 9, 2017

djreiss Aug 4, 2017

djreiss Aug 4, 2017

djreiss Aug 4, 2017

djreiss Aug 4, 2017

mrawls commented Aug 10, 2017

djreiss left a comment

djreiss Aug 10, 2017

mrawls Aug 10, 2017

djreiss Aug 10, 2017

djreiss Aug 10, 2017

kfindeisen Aug 10, 2017

mrawls Aug 10, 2017

DM-11021: Generalize prototype pipeline script #4

DM-11021: Generalize prototype pipeline script #4

Conversation

mrawls commented Jul 28, 2017

kfindeisen Jul 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djreiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrawls commented Aug 10, 2017

djreiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Jul 31, 2017 •

edited

kfindeisen Aug 7, 2017 •

edited