Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tickets/dm 11653 #70

Merged
merged 4 commits into from
Oct 3, 2017
Merged

Tickets/dm 11653 #70

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83db500
Apply autopep8
mfisherlevine Sep 27, 2017
d8beb32
Apply futurize -1 -n -w .
mfisherlevine Sep 27, 2017
75ba982
Run futurize -2 -x division_safe -n -w on yamlCamera.py
mfisherlevine Sep 28, 2017
3b7075d
Add python3 imports
mfisherlevine Sep 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion python/lsst/obs/base/cameraMapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1176,7 +1176,7 @@ def _initWriteRecipes(self):
recipes.update(overrides)

self._writeRecipes = {}
validationMenu = {'FitsStorage': validateRecipeFitsStorage,}
validationMenu = {'FitsStorage': validateRecipeFitsStorage, }
for storageType in recipes.names(True):
if "default" not in recipes[storageType]:
raise RuntimeError("No 'default' recipe defined for storage type %s in %s" %
Expand Down
2 changes: 1 addition & 1 deletion python/lsst/obs/base/mapping.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ def map(self, mapper, dataId, write=False):
addFunc = getattr(mapper, addFunc)
additionalData = addFunc(self.datasetType, actualId)
assert isinstance(additionalData, PropertySet), \
"Bad type for returned data: %s" (type(additionalData),)
"Bad type for returned data: %s" (type(additionalData),)
else:
additionalData = None

Expand Down
8 changes: 5 additions & 3 deletions python/lsst/obs/base/yamlCamera.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
# the GNU General Public License along with this program. If not,
# see <http://www.lsstcorp.org/LegalNotices/>.
#
from __future__ import division
from builtins import zip
import yaml

import numpy as np
Expand Down Expand Up @@ -47,7 +49,7 @@ def __init__(self, cameraYamlFile):
fieldAngleToFocalPlane = afwGeom.makeRadialTransform(radialCoeffs)
focalPlaneToFieldAngle = fieldAngleToFocalPlane.getInverse()
cameraTransformMap = cameraGeom.TransformMap(cameraGeom.FOCAL_PLANE,
{cameraGeom.FIELD_ANGLE: focalPlaneToFieldAngle})
{cameraGeom.FIELD_ANGLE: focalPlaneToFieldAngle})
detectorList = self._makeDetectorList(cameraParams["CCDs"], focalPlaneToFieldAngle)
cameraGeom.Camera.__init__(self, cameraParams["name"], detectorList, cameraTransformMap)

Expand Down Expand Up @@ -110,13 +112,13 @@ def _makeAmpInfoCatalog(self, ccd):
"""
# Much of this will need to be filled in when we know it.
assert len(ccd['amplifiers']) > 0
amp = ccd['amplifiers'].values()[0]
amp = list(ccd['amplifiers'].values())[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont know how big the ccd dictionary is, but most of the time you want to avoid creating a whole list object just to get the first element, as that can be expensive and will be thrown away right away. Something like:
amp = next(iter(ccd['amplifiers'].values())) would be better. However be sure that this is how you actually want to grab the amp. If this is an unordered dict, if someone changes something in the future 0 might not be the correct index. I dont know specifically what you are trying to do here, so this is just more of a general knowledge comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iter won't be needed on python3 so it's one of those code cleanups we'll have to look out for next spring when we drop python2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who added the list? Is it needed, or is it someone worrying about changes in iterators in py2/py3 and we can just use ccd['amplifiers'].values()[0] now and forever?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't index into values() on python3 because it's an iterator. You need to turn it into a list if you want to select specific entries.

Copy link
Contributor

@natelust natelust Oct 3, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tim, values() of a dictionary is not a generator, it is a dictionary_values object which is just a dictionary view, which is why the iter is necessary even on python 3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I always miss that dict_values() thing.

>>> d = {1: 2, 3: 4, 5: 6}
>>> d.values()
dict_values([2, 4, 6])
>>> d.values()[0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'dict_values' object does not support indexing
>>> next(d.values())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'dict_values' object is not an iterator
>>> next(iter(d.values()))
2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew it was a generator but assumed that they'd added the syntactic sugar to make this work.

The code doesn't care that it's the first amp, it just wants one so ordering is unimportant

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Values gives you an object that will stay up to date if someone modifies the original dictionary later, which can be useful. However in this case it seems that Merline only wants the first value for whatever definition of first he is choosing (if this is not an ordered dict).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the length of this list will likely never be over 16 elements, I will leave as-is for clarity, but I take the point for future uses if/when large lists might be in use.


rawBBox = self._makeBBoxFromList(amp['rawBBox']) # total in file
xRawExtent, yRawExtent = rawBBox.getDimensions()

from lsst.afw.table import LL, LR, UL, UR
readCorners = dict(LL = LL, LR = LR, UL = UL, UR = UR)
readCorners = dict(LL=LL, LR=LR, UL=UL, UR=UR)

schema = AmpInfoTable.makeMinimalSchema()

Expand Down
6 changes: 4 additions & 2 deletions tests/camera/camera.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
config.transformDict.transforms['FieldAngle'].transform['affine'].translation = [0.0, 0.0]
config.transformDict.transforms['FieldAngle'].transform['affine'].linear = [1.0, 0.0, 0.0, 1.0]
config.transformDict.transforms['FieldAngle'].transform['radial'].coeffs = None
config.transformDict.transforms['FieldAngle'].transform['inverted'].transform.retarget(target=lsst.afw.geom.transformRegistry['radial'])
config.transformDict.transforms['FieldAngle'].transform['inverted'].transform.coeffs = [0.0, 14805.4, 13619.3, 426637.0]
config.transformDict.transforms['FieldAngle'].transform['inverted'].transform.retarget(
target=lsst.afw.geom.transformRegistry['radial'])
config.transformDict.transforms['FieldAngle'].transform['inverted'].transform.coeffs = [
0.0, 14805.4, 13619.3, 426637.0]
config.transformDict.transforms['FieldAngle'].transform.name = 'inverted'
config.detectorList = {}
config.detectorList[0] = lsst.afw.cameraGeom.cameraConfig.DetectorConfig()
Expand Down
6 changes: 5 additions & 1 deletion tests/test_cameraMapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
# see <http://www.lsstcorp.org/LegalNotices/>.
#

from __future__ import print_function
from builtins import range
import collections
import gc
Expand Down Expand Up @@ -58,6 +59,7 @@ def __init__(self):
def getPackageDir(cls):
return "/path/to/nowhere"


class MinMapper1(lsst.obs.base.CameraMapper):
packageName = 'larry'

Expand Down Expand Up @@ -211,7 +213,7 @@ def testGetDatasetTypes(self):
"someCatalog", "someCatalog_md", "someCatalog_filename",
"someCatalog_len", "someCatalog_schema",
"forced_src", "forced_src_md", "forced_src_filename",
"forced_src_len","forced_src_schema",
"forced_src_len", "forced_src_schema",
"other_sub", "other_filename", "other_md", "other",
"someGz", "someGz_filename", "someFz", "someFz_filename", "someGz_md",
"someFz_sub", "someFz_md", "someGz_sub",
Expand Down Expand Up @@ -442,6 +444,7 @@ def testPackageName(self):
with self.assertRaises(ValueError):
MinMapper3.getPackageName()


class ParentRegistryTestCase(unittest.TestCase):

@staticmethod
Expand Down Expand Up @@ -499,6 +502,7 @@ def test(self):
registryB = butler._repos.outputs()[0].repo._mapper.registry
self.assertNotEqual(id(registryA), id(registryB))


class MissingPolicyKeyTestCase(unittest.TestCase):

def testGetRaises(self):
Expand Down