Fix obs_comCam from not-working to working-to-first-order. #2

mfisherlevine · 2017-07-20T15:45:53Z

The commit includes fixes to:
Fix retargetting of task.
Change ccd datatype from int to text
Add wavelength translator and fix up the ingest
Add config files for calib construction
Add ccd number to template for calibs
Add call to makeRawVisitInfo.getDarkTime() for getting the darkTime correctly
Remove errant .gz suffixes from the policy file

parejkoj

See comments.

parejkoj · 2017-07-27T20:08:27Z

python/lsst/obs/comCam/comCamMapper.py

+        argDict["darkTime"] = self.popFloat(md, "DARKTIME")
+        #
+        # Done setting argDict; check values now that all the header keywords have been consumed
+        #


No need for enclosing empty comment lines.

Yeah, honestly, that was Robert's comment, and he always does that, I don't know why. Removed.

parejkoj · 2017-07-27T20:11:33Z

python/lsst/obs/comCam/ingest.py

+
+        Returns
+        -------
+        phuInfo : `dict`


Can we not call this phuInfo, please? How about primaryHDUDict?

I'm happy to change this, but this was a conscious decision to adhere to the same naming scheme as the base class, and to be the same as other ingest.getInfo() methods in other obs_packages. Would you really like me to change this if it means no longer having that symmetry?

Also, give that these objects are instantiated by a call to the base class (first line), keeping the same variable names as those returned seems neat, and doing otherwise might cause more confusion than using better names would save, IMO.

I agree with you both here: this name is terrible, but consistency is important. My take is that the latter wins for the purposes of this ticket.

Ugh. Can you file a ticket to fix the name in the base class then, please? Consistency is ok for this work, but that name is terrible.

Ugh indeed. See DM-11428 for the ticket that was just born though 🙂

parejkoj · 2017-07-27T20:12:18Z

python/lsst/obs/comCam/ingest.py

+        phuInfo : `dict`
+            Dictionary containing the header keys defined in the ingest config from the primary HDU
+        infoList : `list`
+            A list of dictionaries containing the phuInfo(s)


I'm confused how this is different from the above (and also, phuInfo name again...).

In the case of single extension files, it's a dict, and a [dict], otherwise the second object is a list of dictionaries containing header data. I agree it's not very nice, but again, it's inheriting this structure from the base class. All this should be cleaned up in the Great Obs Package Redesign, but for now I think this is how it has to be 😔

Can we at least document this better? I wouldn't have been able to derive (at least, not with any certainty) your explanation above from looking at the docstring.

Yes, what other John said.

OK, I think my new docstring should make this clear. Shout if you disagree (and let's hope it's temporary anyway)

parejkoj · 2017-07-27T20:12:58Z

python/lsst/obs/comCam/ingest.py

        phuInfo, infoList = ParseTask.getInfo(self, filename)

        pathname, basename = os.path.split(filename)
        basename = re.sub(r"\.(%s)$" % "|".join(EXTENSIONS), "", basename)
        phuInfo['basename'] = basename
        #
-        # Now pull the sensor ID from the path (no, it's not in the header)
+        # Now pull the acq type & jobID from the path (no, they're not in the header)


"acq type"? jobID? Might be worth a note about what these are.

Good point, added.

parejkoj · 2017-07-27T20:14:29Z

python/lsst/obs/comCam/ingest.py

-        phuInfo['ccd'] = sensorLocationInRaft
+        phuInfo['raftId'] = raftId # also in the header - RAFTNAME
+        phuInfo['field'] = acquisitionType # NOT in the header
+        phuInfo['jobId'] = int(jobId) #  NOT in the header, but do we need it?


Have you determined whether we need the "do we need it"s? If we don't, drop them, if you have reason to believe they'll be useful in the future, just remove the comments.

I thought it might be useful to flag these so that I could check back in future to see if they were being used to de-cruft the registry, but perhaps you're right, and I should just de-cruft the code now. Removed 👍

parejkoj · 2017-07-27T20:16:08Z

python/lsst/obs/comCam/ingest.py

+        Typically the position is within 0.005nm of the desired position, so we warn if it's not very
+        close to an integer value.
+
+        Future users should be aware that the HIERARCH MONOCH-WAVELENG key is NOT the requested value, and


Useful comment: glad you documented it.

parejkoj · 2017-07-27T20:17:24Z

python/lsst/obs/comCam/ingest.py

+        wl = int(round(raw_wl))
+        if abs(raw_wl-wl)>=0.1:
+            logger = lsstLog.Log.getLogger('obs.comCam.ingest')
+            logger.warn('Translated signigicatly non-integer wavelength %s', raw_wl)


"significantly"

Is a log warning enough if it's not an int? Should it raise instead? What happens downstream?

I'm guessing your "significantly" comment is an objection to the lack of precision, given that they're ~always going to be non-integer to some extent. I've changed that to a more quantitative statement, using the 0.1nm constant that I'm testing against. Let me know if that's not what you wanted to imply.

For the warning, I think that's the right thing to do. The value is not necessarily wrong, and if downstream it turns into garbage then you want to just see a warning and what goes wrong, rather than it raising at the time.

I'm guessing his "significantly" comment is related to "signigicatly" not being a word. :-)

Lol, good point, thanks John! The (other) all-seeing eye strikes again! I've still changed the comment to something more rigorous though 🙂

parejkoj · 2017-07-28T19:52:25Z

python/lsst/obs/comCam/ingest.py

@@ -84,7 +85,7 @@ def translate_wavelength(self, md):
        wl = int(round(raw_wl))
        if abs(raw_wl-wl)>=0.1:
            logger = lsstLog.Log.getLogger('obs.comCam.ingest')
-            logger.warn('Translated signigicatly non-integer wavelength %s', raw_wl)
+            logger.warn('Translated overly non-integer wavelength; %s is more than 0.1nm from an integer value', raw_wl)


"Overly" is overly worse, I think. "significantly" (spelled correctly) is fine, particularly with the latter added comment.

The commit includes fixes to: Fix retargetting of task. Change ccd datatype from int to text Add wavelength translator and fix up the ingest Add config files for calib construction Add ccd number to template for calibs Add call to makeRawVisitInfo.getDarkTime() for getting the darkTime correctly Remove errant .gz suffixes from the policy file

parejkoj reviewed Jul 27, 2017

View reviewed changes

parejkoj reviewed Jul 28, 2017

View reviewed changes

mfisherlevine force-pushed the tickets/DM-9872 branch from 3917ff5 to 1399a01 Compare August 1, 2017 12:22

mfisherlevine force-pushed the tickets/DM-9872 branch from 1399a01 to 7875a13 Compare August 1, 2017 12:23

mfisherlevine merged commit b330c95 into master Aug 1, 2017

ktlim deleted the tickets/DM-9872 branch August 25, 2018 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix obs_comCam from not-working to working-to-first-order. #2

Fix obs_comCam from not-working to working-to-first-order. #2

mfisherlevine commented Jul 20, 2017

parejkoj left a comment

parejkoj Jul 27, 2017

mfisherlevine Jul 28, 2017

parejkoj Jul 27, 2017

mfisherlevine Jul 28, 2017

mfisherlevine Jul 28, 2017

jdswinbank Jul 28, 2017

parejkoj Jul 28, 2017

mfisherlevine Jul 31, 2017

parejkoj Jul 27, 2017

mfisherlevine Jul 28, 2017

jdswinbank Jul 28, 2017

parejkoj Jul 28, 2017

mfisherlevine Jul 31, 2017

parejkoj Jul 27, 2017

mfisherlevine Jul 28, 2017

parejkoj Jul 27, 2017

mfisherlevine Jul 28, 2017

parejkoj Jul 27, 2017

parejkoj Jul 27, 2017

mfisherlevine Jul 28, 2017

jdswinbank Jul 28, 2017

mfisherlevine Jul 28, 2017

parejkoj Jul 28, 2017

Fix obs_comCam from not-working to working-to-first-order. #2

Fix obs_comCam from not-working to working-to-first-order. #2

Conversation

mfisherlevine commented Jul 20, 2017

parejkoj 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment