port obs_subaru to python3 Tickets/dm 7306 #42
Conversation
| @@ -62,7 +66,7 @@ def __init__(self, path, version, validStart, validEnd=None): | |||
| # Fix up end dates so there are no collisions. | |||
| # Defects files for a CCD are valid from the date they are registered until the next date. | |||
| # This means that any defects file should carry ALL the defects that are present at that time. | |||
| for ccd, rowList in rowsPerCcd.iteritems(): | |||
| for ccd, rowList in rowsPerCcd.items(): | |||
| rowList.sort(key=lambda row: row.validStart) # ISO-8601 will sort just fine without conversion from str | |||
There was a problem hiding this comment.
Could this be turned into a list comprehension syntax instead of lambda? Would that be faster/more pythonic?
There was a problem hiding this comment.
For reference, I think this would be a more modern version:
from operator import attrgetter
rowList.sort(key=attrgetter('validStart'))But since this executable code doesn't have any tests, I'm loathe to change it.
|
|
||
| print ccdId, ccdBoundary, amp1Boundary, amp2Boundary, gain1, gain2, rdnoise1, rdnoise2, \ | ||
| saturate1, saturate2 | ||
| print(ccdId, ccdBoundary, amp1Boundary, amp2Boundary, gain1, gain2, rdnoise1, rdnoise2, \ |
There was a problem hiding this comment.
The line continuation \ isn't needed here anymore
There was a problem hiding this comment.
I rewrapped that at a better place.
| 108 : 110, | ||
| 114 : 108, | ||
| 108: 110, | ||
| 114: 108, | ||
| } |
There was a problem hiding this comment.
The spacing choices here are confusing me... not sure if these should be treated more like indices, i.e., 100:101 or new assignments, i.e., 100: 101.
There was a problem hiding this comment.
They're assignments: it's a dict. I cleaned up the commented lines, though.
|
|
||
| plots, labels = [], [] | ||
| for v, r, d in zip(visits, ra, dec): | ||
| field = fields.get(v) | ||
| if verbose: | ||
| print "Drawing %s %s \r" % (v, field), | ||
| print("Drawing %s %s \r" % (v, field), end=' ') |
There was a problem hiding this comment.
Is having both the end=' ' and flushing stdout desired here? I'm guessing yes, since this happens again a bit later.
There was a problem hiding this comment.
Those are different things: putting end=' ' adds a space (like the hanging comma did in python2), while stdout.flush() flushes the buffer (very useful in a multi-threaded environment, or if other things are processing).
There was a problem hiding this comment.
Although, now that you mention it: python3's print() has a flush=True argument. I'll just use that instead.
There was a problem hiding this comment.
In that case, there are a few other places where stdout is flushed, so maybe you could quickly apply that there too?
There was a problem hiding this comment.
I only see two in showDither.py, which I've done.
| @@ -329,7 +339,8 @@ def frameInfoFrom(filepath): | |||
| if mat: | |||
| v1 = int(mat.group(1)) | |||
| v2 = int(mat.group(2)) | |||
| v3 = mat.group(3); v3 = int(v3) if v3 else 1 | |||
| v3 = mat.group(3) | |||
| v3 = int(v3) if v3 else 1 | |||
There was a problem hiding this comment.
Debatable if having this on two lines actually improves clarity or not; perhaps do v3 = int(mat.group(3)) if mat.group(3) else 1 ?
There was a problem hiding this comment.
Hmm... That could probably go either way. Sadly, it's another untestable bin/ file that I'd rather not muck around in.
| @@ -946,7 +980,7 @@ def mosaicIo(dirName, mosaics=None, mode=None): | |||
| if mode == "r": | |||
| import glob | |||
| for f in glob.glob(os.path.join(dirName, "*.fits")) + \ | |||
| glob.glob(os.path.join(dirName, "*.fits.gz")): | |||
| glob.glob(os.path.join(dirName, "*.fits.gz")): | |||
There was a problem hiding this comment.
Parentheses preferred to line continuation \ character
There was a problem hiding this comment.
I've pulled the globbing into a "files" variable.
| visit & median flux & exposure time & line type & line colour \\ | ||
| \hline""" | ||
| \hline""") |
There was a problem hiding this comment.
The indentations here are confusing
There was a problem hiding this comment.
It's doing something with latex and a raw string.
There was a problem hiding this comment.
Right, but can't each portion of the print statement still be consistently indented? Or does that actually confuse latex?
There was a problem hiding this comment.
No: raw strings are raw with all formatting intact, so they have to look like this for latex (as far as I'm aware).
| print r"""\caption{\label{%sLegend} Index of lines in %s radial profiles} | ||
| \end{longtable}""" % (filterName, filterName) | ||
| print(r"""\caption{\label{%sLegend} Index of lines in %s radial profiles} | ||
| \end{longtable}""" % (filterName, filterName)) |
There was a problem hiding this comment.
More confusing indentations
There was a problem hiding this comment.
More latex raw strings.
| 'green' if v in domeflats else \ | ||
| 'black' if v in science else \ | ||
| 'red' if v in skyflats else \ | ||
| 'blue' |
There was a problem hiding this comment.
This is messy, but I can't think of a better way to do it offhand.
| if tract < 0 or tract >= 128: | ||
| raise RuntimeError('tract not in range [0,128)') | ||
| patchX, patchY = map(int, dataId['patch'].split(',')) | ||
| patchX, patchY = list(map(int, dataId['patch'].split(','))) |
There was a problem hiding this comment.
Another possibly less-than-desirable list(map()) situation
There was a problem hiding this comment.
and it can be a generator because the result is immediately unpacked.
Leaving as a separate commit as it'll be hard to rebase into the various other pep8/futurize steps.
No description provided.