Tickets/DM 7756 #19

r-owen · 2017-02-09T23:02:20Z

Make the code compatible with both python 2 and python 3
Modernize the code and make it mostly pep8 compliant (a few list comprehensions still redefine a local variable).

and perform a few trivial manual cleanups. There are a few more substantial warnings that were left alone (several list comprehensions that redefine a variable and several instances of assigning a variable to a lambda expression)

timj · 2017-02-09T23:03:58Z

python/lsst/pipe/drivers/coaddDriver.py

-        lookup = dict(zip(map(refNamer, patchRefList), selectedData))
+        refNamer = lambda patchRef: tuple(
+            map(int, patchRef.dataId["patch"].split(",")))
+        lookup = dict(list(zip(list(map(refNamer, patchRefList)), selectedData)))


I don't think these list( calls are needed here as the lists are transient.

timj · 2017-02-09T23:06:03Z

python/lsst/pipe/drivers/constructCalibs.py

@@ -244,28 +271,40 @@ def parse_args(self, *args, **kwargs):

        keys = namespace.butler.getKeys(self.calibName)
        parsed = {}
-        for name, value in namespace.calibId.items():
+        for name, value in list(namespace.calibId.items()):


Remove the list() wrapper.

timj · 2017-02-09T23:06:26Z

python/lsst/pipe/drivers/constructCalibs.py


        # Ensure we can generate filenames for each output
        for ccdName in ccdIdLists:
-            dataId = dict(outputId.items() + [(k, ccdName[i]) for i, k in enumerate(self.config.ccdKeys)])
+            dataId = dict(list(outputId.items()) + [(k, ccdName[i])


list() not needed.

timj · 2017-02-09T23:06:44Z

python/lsst/pipe/drivers/constructCalibs.py

@@ -415,16 +463,17 @@ def scatterProcess(self, pool, ccdIdLists):
        @param ccdIdLists  Dict of data identifier lists for each CCD name
        @return Dict of lists of returned data for each CCD name
        """
-        dataIdList = sum(ccdIdLists.values(), [])
+        dataIdList = sum(list(ccdIdLists.values()), [])


I don't think list() is needed here.

timj · 2017-02-09T23:06:58Z

python/lsst/pipe/drivers/constructCalibs.py

        indices = dict(sum([[(tuple(dataId.values()) if dataId is not None else None, (ccdName, expNum))
                             for expNum, dataId in enumerate(expList)]
-                            for ccdName, expList in ccdIdLists.items()], []))
+                            for ccdName, expList in list(ccdIdLists.items())], []))


Remove list()

timj · 2017-02-09T23:08:10Z

python/lsst/pipe/drivers/constructCalibs.py

        self.log.info("Scatter processing")

        resultList = pool.map(self.process, dataIdList)

        # Piece everything back together
-        data = dict((ccdName, [None] * len(expList)) for ccdName, expList in ccdIdLists.items())
+        data = dict((ccdName, [None] * len(expList))
+                    for ccdName, expList in list(ccdIdLists.items()))


Remove list()

timj · 2017-02-09T23:08:35Z

python/lsst/pipe/drivers/constructCalibs.py

@@ -518,7 +568,7 @@ def scale(self, ccdIdLists, data):
        """
        self.log.info("Scale on %s" % NODE)
        return dict((name, Struct(ccdScale=None, expScales=[None] * len(ccdIdLists[name])))
-                    for name in ccdIdLists.keys())
+                    for name in list(ccdIdLists.keys()))


Remove list() and keys()

timj

There are many list() and keys() calls that can be removed.

timj · 2017-02-09T23:08:48Z

python/lsst/pipe/drivers/constructCalibs.py

@@ -535,9 +585,9 @@ def scatterCombine(self, pool, outputId, ccdIdLists, scales):
        """
        self.log.info("Scatter combination")
        data = [Struct(ccdIdList=ccdIdLists[ccdName], scales=scales[ccdName],
-                       outputId=dict(outputId.items() +
+                       outputId=dict(list(outputId.items()) +


Remove list()

timj · 2017-02-09T23:09:15Z

python/lsst/pipe/drivers/constructCalibs.py

                                     [(k, ccdName[i]) for i, k in enumerate(self.config.ccdKeys)])) for
-                ccdName in ccdIdLists.keys()]
+                ccdName in list(ccdIdLists.keys())]


Remove list() and keys()

timj · 2017-02-09T23:09:58Z

python/lsst/pipe/drivers/constructCalibs.py


        return dict((ccdName, Struct(ccdScale=compScales[indices[ccdName]], expScales=expScales))
-                    for ccdName in ccdIdLists.keys())
+                    for ccdName in list(ccdIdLists.keys()))


Remove list() and keys().

timj · 2017-02-09T23:11:01Z

python/lsst/pipe/drivers/coaddDriver.py

-        key = lambda dataRef: tuple(dataRef.dataId[k] for k in sorted(dataRef.dataId.keys()))
-        inputs = dict((key(select.dataRef), select) for select in selectDataList)
+        key = lambda dataRef: tuple(
+            dataRef.dataId[k] for k in sorted(dataRef.dataId.keys()))


I think sorted(dataRef.dataId) should work if this is dict-like.

timj · 2017-02-09T23:11:49Z

python/lsst/pipe/drivers/constructCalibs.py

-        for name in ccdIdLists.keys():
+        bgMatrix = np.array([[0.0] * len(expList)
+                            for expList in list(ccdIdLists.values())])
+        for name in list(ccdIdLists.keys()):


remove list() and keys()

timj

These changes look reasonable to me.

...in order to make the code PEP8 compliant and easier to read.

This simply changed two instances of `catch Exception, e:` to `catch Exception as e:`

Modernize with futurize -2 Group imports properly Change `import numpy` to `import numpy as np` Add `# noqa` to one line added by futurize to hide a pep8 warning

futurize added some unnecessary `list(...)` for safety; I removed the ones that are not required.

...to avoid redefining local variables

Run autopep8 on the code

9011194

and perform a few trivial manual cleanups. There are a few more substantial warnings that were left alone (several list comprehensions that redefine a variable and several instances of assigning a variable to a lambda expression)

timj reviewed Feb 9, 2017

View reviewed changes

timj approved these changes Feb 10, 2017

View reviewed changes

r-owen added 6 commits February 10, 2017 15:58

Turn foo = lambda into def foo

730c23a

...in order to make the code PEP8 compliant and easier to read.

Modernize the code using futurize -1

a8659a5

This simply changed two instances of `catch Exception, e:` to `catch Exception as e:`

Make code py2 and py3 compatible

882a02a

Modernize with futurize -2 Group imports properly Change `import numpy` to `import numpy as np` Add `# noqa` to one line added by futurize to hide a pep8 warning

remove unneeded list(...)

53e1ab4

futurize added some unnecessary `list(...)` for safety; I removed the ones that are not required.

Remove unneeded .keys()

bb56eb4

Rename variables in list comprehensions

0c22efb

...to avoid redefining local variables

r-owen force-pushed the tickets/DM-7756 branch from 630dfa6 to 0c22efb Compare February 11, 2017 00:04

r-owen merged commit 0c22efb into master Feb 11, 2017

ktlim deleted the tickets/DM-7756 branch August 25, 2018 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tickets/DM 7756 #19

Tickets/DM 7756 #19

r-owen commented Feb 9, 2017

timj Feb 9, 2017

timj Feb 9, 2017

timj Feb 9, 2017

timj Feb 9, 2017

timj Feb 9, 2017

timj Feb 9, 2017

timj Feb 9, 2017 •

edited

timj left a comment

timj Feb 9, 2017

timj Feb 9, 2017

timj Feb 9, 2017

timj Feb 9, 2017

timj Feb 9, 2017

timj left a comment

Tickets/DM 7756 #19

Tickets/DM 7756 #19

Conversation

r-owen commented Feb 9, 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

timj Feb 9, 2017 • edited

Choose a reason for hiding this comment

timj 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

timj left a comment

Choose a reason for hiding this comment

timj Feb 9, 2017 •

edited