port obs_decam to python3 Tickets/dm 7307 #37
Conversation
There was a problem hiding this comment.
Goodness, there were so many changes in camera.py that GitHub can't display it? Fun. Was that just a bunch of PEP 8 stuff too, or something more substantial?
Edit... I realized I can answer my own question by looking at each individual commit... and it is all PEP 8 updates to a laundry list of manually incremented values, so I'm confident it's fine.
| if tract < 0 or tract >= 2**DecamMapper._nbit_tract: | ||
| raise RuntimeError('tract not in range [0,%d)' % (2**DecamMapper._nbit_tract)) | ||
| patchX, patchY = map(int, dataId['patch'].split(',')) | ||
| patchX, patchY = [int(x) for x in dataId['patch'].split(',')] |
There was a problem hiding this comment.
Assuming these are indeed equivalent, I learned a thing about map and list comprehensions today, thanks!
There was a problem hiding this comment.
Yes, map functionality can be done with list comprehensions, and it's usually easier to read. There was once a speed difference (circa python 2.4), but that's no longer the case.
| @@ -1,3 +1,4 @@ | |||
| from __future__ import absolute_import, division, print_function | |||
There was a problem hiding this comment.
It's clearer to have one import statement per line.
There was a problem hiding this comment.
I think the one exception we make to the "one import per line" is for future, since we generally want all three. @r-owen suggested always having that line in alphabetical order, to make for easier searching in the future.
| @@ -58,6 +58,7 @@ def setUp(self): | |||
| "isr.assembleCcd.setGain=False", | |||
| # This test uses CP-MasterCal calibration products | |||
| "-C", "%s/processCcdCpIsr.py" % configPath] | |||
| argsList.append('--doraise') | |||
There was a problem hiding this comment.
Can you clarify what this line is adding? Was testProcessCcd failing silently before in some situations?
There was a problem hiding this comment.
Good point! Should I clarify in the commit?
Your guess is almost correct: previously, ProcessCcdTask.parseAndRun was failing without raising an exception, and so the subsequent tests would fail because the files they expected were not created, and thus it was really unclear where the problem was.
There was a problem hiding this comment.
That makes sense. It wouldn't hurt to elaborate in the commit in case folks want an example of how to add clearer exceptions when troubleshooting mystery failures.
There was a problem hiding this comment.
I've pushed an updated commit message, based on the above. What do you think?
ProcessCcdTask.parseAndRun was failing without raising an exception, and so the subsequent tests would fail because the files they expected were not created, and thus it was really unclear where the problem was. Adding --doraise to the arguments means the failure happens in setUp, so debugging is easier.
parejkoj
force-pushed
the
tickets/DM-7307
branch
from
73ae1db
to
770a1f1
Compare
No description provided.