Review for DM-10253: make obs_ctio0m9 work #4

jdswinbank · 2017-06-08T14:19:49Z

No description provided.

SimonKrughoff · 2017-06-09T22:58:33Z

python/lsst/obs/ctio0m9/ctio0m9Mapper.py

 from lsst.obs.ctio0m9 import Ctio0m9

 class Ctio0m9MakeRawVisitInfo(MakeRawVisitInfo):
    """functor to make a VisitInfo from the FITS header of a raw image
    """
-
+    


White space only change.

SimonKrughoff · 2017-06-09T22:59:47Z

python/lsst/obs/ctio0m9/ctio0m9Mapper.py

    def setArgDict(self, md, argDict):
        """Fill an argument dict with arguments for makeVisitInfo and pop associated metadata
        """
        super(Ctio0m9MakeRawVisitInfo, self).setArgDict(md, argDict)
+        argDict["darkTime"] = md.get("DARKTIME")
+        #The line above should maybe be moved to obs_base/makeRawVisitInfo.setArgDict() and read:


If you think it should be moved there, can you file a ticket to that effect please?

Filed, see DM-10944. Comment removed.

SimonKrughoff · 2017-06-09T23:01:29Z

python/lsst/obs/ctio0m9/ctio0m9Mapper.py

+        """Hook to retrieve number of bits in identifier for CCD"""
+        return 32
+
+    def std_raw_md(self, md, dataId):


As stated in the ticket, I think this should actually call the same free function that translate_taiDate uses so that we do not need the change to pipe_tasks.

SimonKrughoff · 2017-06-09T23:06:08Z

python/lsst/obs/ctio0m9/ingest.py

@@ -107,7 +168,15 @@ def _translateFromCalibId(self, field, md):
        return match.groups()[0]

    def translate_filter(self, md):
+        print('\nMerlin says there might be a problem here\nCheck what is happening when you see this\n')


This should be a {{log.warn}} message. And maybe include the commented information below.

This should never have been left in! This was just a note-to-self, and has been removed. Apologies.

SimonKrughoff · 2017-06-09T23:07:28Z

scripts/pre_ingest_sanitisation.py

+#################################
+# Set these two variables, then rest should run:
+source_path = '/nfs/lsst2/photocalData/data/ctio0m9/'
+dest_root_path = '/nfs/lsst2/photocalData/data/ctio0m9_sanitised/'


This would be much better as arguments using argparse

SimonKrughoff · 2017-06-09T23:08:08Z

scripts/pre_ingest_sanitisation.py

@@ -0,0 +1,236 @@
+# For an annotated version, use the ipython notebook.


I'm worried about keeping things in sync. If this is not auto generated from the ipynb, it could easily drift away from what is in there.

It's not, I made this myself because I thought that having it only exist as an IPython notebook might make people sad, so I made this for convenience, but I take your point about only having one. Given that I find having it as a notebook very useful for future poking I'm going to just delete the script.

SimonKrughoff · 2017-06-09T23:12:00Z

scripts/pre_ingest_sanitisation.py

+    headers[filename] = main_header
+    if i%500==0: print 'loaded %s of %s'%(i, nfiles)
+
+print 'Finished loading headers'


If you are sanitizing headers here too, is it reasonable to fix the dates here?

Oh I guess you don't want to modify the files?

O.K. I'm back to my first question. If you are messing with the files on disk that are actually processed, why not just make sure the dates are sane in the files you process? That would mean the sanitization code only lives in one place.

SimonKrughoff

I think we have to come up with an alternate solution to the observation date sanitization. I'd be happy with either doing it in pre-ingest or include it in both the ingest translator and in std_raw_md. If the latter, it needs to only appear in one place.

SimonKrughoff · 2017-06-12T21:58:35Z

scripts/pre_ingest_sanitisation.py

+
+    dt = dafBase.DateTime(date_obs, dafBase.DateTime.TAI)
+    mjd = dt.get(dafBase.DateTime.MJD) # MJD is actually the default
+    mmjd = mjd - 55197              # relative to 2010-01-01, just to make the visits a tiny bit smaller


This seems arbitrary and makes the visits numbers carry less information. If you are going to change the zernpoint, why not change to something closer to the beginning of observations? Aren't these data all taken the last year or so?

SimonKrughoff · 2017-06-12T21:59:58Z

scripts/pre_ingest_sanitisation.py

+    this_header = this_file[0].header
+    old_object = this_header['OBJECT']
+    if old_object in conversion.keys():
+#         print 'would have changed %s to %s'%(old_object, conversion[old_object])


I prefer not to see commented out code.

SimonKrughoff · 2017-06-12T22:02:26Z

scripts/pre_ingest_sanitisation.py

+    headers[filename] = main_header
+    if i%500==0: print 'loaded %s of %s'%(i, nfiles)
+
+print 'Finished loading headers'


O.K. I'm back to my first question. If you are messing with the files on disk that are actually processed, why not just make sure the dates are sane in the files you process? That would mean the sanitization code only lives in one place.

SimonKrughoff · 2017-06-12T22:03:56Z

scripts/pre_ingest_sanitisation.py

+            TZ = ""
+
+        date_obs = "%4d-%02d-%02dT%02d:%02d:%06.3f%s" % (int(year), int(month), int(day),
+                                                         int(h),    int(m),     float(s), TZ)


If you are going to do sanitization without writing it to the headers, I'd much prefer that this code be put in a free function in the library path and called from there rather than having it in two places.

SimonKrughoff · 2017-06-12T22:04:04Z

scripts/pre_ingest_sanitisation.py

+        headers[filename]['IMAGETYP'] = 'OBJECT' # reflect patch in our dict
+        this_file.flush()
+        this_file.close()
+        print 'Pathced header in %s'%filename


Pathced -> Patched

SimonKrughoff · 2017-06-12T22:05:35Z

scripts/run_all.sh

+
+echo "lsst.obs.ctio0m9.Ctio0m9Mapper" > $REPO_DIR/_mapper
+
+# Ignore these entirely - I think these are all duplicates, or have the wrong size CCD


Please check on these cases and delete this comment before committing to master.

This script to rerun things doesn't really belong on the obs package, but I don't know a better place for it to live, and we don't want to create a new package for such things. I have made the comment less tentative, so that it accurately describes the state of the data on those paths, and it is no longer commented out code.

I have also renamed it from {{run_all.sh}} to {{run_all_pre_march2017.sh}} as this is more descriptive.

Please let me know if this doesn't fix your concerns here.

SimonKrughoff · 2017-06-12T22:06:54Z

scripts/run_all.sh

+
+#-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
+
+processCcd.py $REPO_DIR --rerun temp_1 --id visit='242612713' -c isr.doLinearize=False isr.doDefect=False


This is a test to see if things worked? If so, please add a note to that effect.

SimonKrughoff · 2017-06-12T22:07:56Z

scripts/run_all.sh

+constructFlat.py $REPO_DIR --rerun $whoami/calibs --id imgType='flat' filter='NONE+i' $batchArgs
+constructFlat.py $REPO_DIR --rerun $whoami/calibs --id imgType='flat' filter='NONE+z' $batchArgs
+constructFlat.py $REPO_DIR --rerun $whoami/calibs --id imgType='flat' filter='NONE+SEMROCK' $batchArgs
+# constructFlat.py $REPO_DIR --rerun $whoami/calibs --id imgType='flat' filter='RONCHI200+SEMROCK' $batchArgs


These are commented out for a reason? If they need to be run sometimes and not other times, can we add a command line argument that allows them to be turned on and off without modifying the file?

Fixed to always be run.

This obs_package is pre-release, and was riddled with bugs/missing code. This large commit fixes all of that up. DM-10565 finishes the job. Fixes include but are not limited to: Update .gitignore Add translator functions for imgType and wavelength Register new columns Add filter aliases Fix whitespace errors and newlines Set default config override files Provide functions for sanitizing metadata Deal with multiple filter wheels update README Fix ingestCalibs Add bypass_ functions Add std_dark() function Add pre-processing scripts Add script to rerun all ingest and calib construction Add nominal CD matrix to all visits at runtime

jdswinbank mentioned this pull request Jun 8, 2017

Review for DM-10565: Get processCcd working with obs_nctio0m9 #5

Merged

SimonKrughoff reviewed Jun 9, 2017

View reviewed changes

SimonKrughoff suggested changes Jun 12, 2017

View reviewed changes

mfisherlevine force-pushed the tickets/DM-10253 branch 2 times, most recently from 2dc83ac to ab50fcb Compare June 22, 2017 16:52

mfisherlevine force-pushed the tickets/DM-10253 branch from ab50fcb to 031ad23 Compare June 22, 2017 17:00

SimonKrughoff approved these changes Jun 26, 2017

View reviewed changes

mfisherlevine merged commit 4ea2fd6 into master Jun 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review for DM-10253: make obs_ctio0m9 work #4

Review for DM-10253: make obs_ctio0m9 work #4

jdswinbank commented Jun 8, 2017

SimonKrughoff Jun 9, 2017

SimonKrughoff Jun 9, 2017

mfisherlevine Jun 15, 2017

SimonKrughoff Jun 9, 2017

SimonKrughoff Jun 9, 2017

mfisherlevine Jun 15, 2017

SimonKrughoff Jun 9, 2017

SimonKrughoff Jun 9, 2017

mfisherlevine Jun 15, 2017

SimonKrughoff Jun 9, 2017

SimonKrughoff Jun 12, 2017

SimonKrughoff left a comment

SimonKrughoff Jun 12, 2017

SimonKrughoff Jun 12, 2017

SimonKrughoff Jun 12, 2017

SimonKrughoff Jun 12, 2017

SimonKrughoff Jun 12, 2017

SimonKrughoff Jun 12, 2017

mfisherlevine Jun 21, 2017

SimonKrughoff Jun 12, 2017

SimonKrughoff Jun 12, 2017

mfisherlevine Jun 21, 2017

		@@ -0,0 +1,236 @@
		# For an annotated version, use the ipython notebook.


		echo "lsst.obs.ctio0m9.Ctio0m9Mapper" > $REPO_DIR/_mapper

		# Ignore these entirely - I think these are all duplicates, or have the wrong size CCD


		#-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

		processCcd.py $REPO_DIR --rerun temp_1 --id visit='242612713' -c isr.doLinearize=False isr.doDefect=False

Review for DM-10253: make obs_ctio0m9 work #4

Review for DM-10253: make obs_ctio0m9 work #4

Conversation

jdswinbank commented Jun 8, 2017

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

SimonKrughoff 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

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