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

Working integration test for CFHT data. #14

Merged
merged 7 commits into from
Aug 2, 2016
Merged

Working integration test for CFHT data. #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
94c04fd
Working integration test for CFHT data.
parejkoj Jul 21, 2016
f8994ec
Post review updates.
parejkoj Jul 28, 2016
1238345
Better visit_list management.
parejkoj Jul 28, 2016
0b478a6
Add profile_jointcal cmdline argument.
parejkoj Jul 29, 2016
bef3ff1
Default to not generate plots.
parejkoj Jul 29, 2016
be8c48a
Normalize histograms and pull common options into a kwarg
parejkoj Jul 29, 2016
7cf737f
Updating based on comments from Ian S.
parejkoj Jul 29, 2016
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
62 changes: 49 additions & 13 deletions python/lsst/jointcal/jointcal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,31 @@ def getTargetList(parsedCmd, **kwargs):
for ref in parsedCmd.id.refList:
refListDict.setdefault(ref.dataId["tract"], []).append(ref)
# we call run() once with each tract
return [(refListDict[tract],
tract
) for tract in sorted(refListDict.keys())]
return [(refListDict[tract],) for tract in sorted(refListDict.keys())]

def __call__(self, args):
"""
@param args Arguments for Task.run()

@return
- None if self.doReturnResults is False
- A pipe.base.Struct containing these fields if self.doReturnResults is True:
- dataRef: the provided data references, with update post-fit WCS's.
"""
task = self.TaskClass(config=self.config, log=self.log)
task.run(*args)
result = task.run(*args)
if self.doReturnResults:
return pipeBase.Struct(result = result)
Copy link
Contributor

Choose a reason for hiding this comment

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

No spaces around = if it's all on one line (e.g., see line 59).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.



class JointcalConfig(pexConfig.Config):
"""Config for jointcalTask"""

coaddName = pexConfig.Field(
doc = "Type of coadd",
dtype = str,
default = "deep"
)
posError = pexConfig.Field(
doc = "Constant term for error on position (in pixel unit)",
dtype = float,
Expand All @@ -77,6 +90,7 @@ class JointcalConfig(pexConfig.Config):
doc = "How to select sources for cross-matching",
default = "astrometry"
)
# TODO: CmdLineTask has a profile thing built-in: can we tie in to that?
Copy link
Contributor

Choose a reason for hiding this comment

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

You should absolutely use the lsst.pipe.base.cmdLineTask.profile context manager.
You should not be setting operational parameters like profiling through Config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, but how does one manage turning on/off profiling in general? I want to be able to enable profiling when timing various things, but I don't want it on by default. Is there a standard approach to that?

Copy link
Member

Choose a reason for hiding this comment

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

We have the lsstDebug trick to handle fine-grained turning on of debugging, and something similar could work for profiling.

In fact, people aren't thrilled about the mechanism that lsstDebug uses, but the replacement for lsstDebug should be able to handle profiling too.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's the --profile command-line argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've dealt with this by adding a "--profile_jointcal" cmdline argument which can be the used with the context manager. I want to be able to profile individual parts of jointcal separately, since I know most of the effort is going into the actual math but there are likely other things that could be sped up independently.

profile = pexConfig.Field(
doc = "Profile jointcal, including the catalog creation step.",
dtype = bool,
Expand All @@ -98,10 +112,7 @@ class JointcalTask(pipeBase.CmdLineTask):
RunnerClass = JointcalRunner
_DefaultName = "jointcal"

def __init__(self, schema, *args, **kwargs):
"""
@param schema source catalog schema for all the catalogs we will work with.
"""
def __init__(self, *args, **kwargs):
pipeBase.CmdLineTask.__init__(self, *args, **kwargs)
self.makeSubtask("sourceSelector")

Expand All @@ -123,7 +134,17 @@ def _makeArgumentParser(cls):
return parser

def _build_ccdImage(self, dataRef, associations, jointcalControl):
"""Extract the necessary things from this dataRef to add a new ccdImage."""
"""
Extract the necessary things from this dataRef to add a new ccdImage.
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a leading ! for doxygen parsing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


@param dataRef (ButlerDataRef) dataRef to extract info from.
@param associations (jointcal.Associations) object to add the info to, to
construct a new CcdImage
@param jointcalControl (jointcal.JointcalControl) control object for
associations management

@return (afw.image.TanWcs) the TAN WCS of this image
"""
src = dataRef.get("src", immediate=True)
md = dataRef.get("calexp_md", immediate=True)
tanwcs = afwImage.TanWcs.cast(afwImage.makeWcs(md))
Expand All @@ -135,7 +156,8 @@ def _build_ccdImage(self, dataRef, associations, jointcalControl):

goodSrc = self.sourceSelector.selectSources(src)

# TODO: NOTE: Leaving the old catalog and comparison code in while we
# ------------------
# TODO: Leaving the old catalog and comparison code in while we
Copy link
Contributor

Choose a reason for hiding this comment

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

Code should not be left commented out. I know you have philosophical disagreements, but I think you should submit to the way things are done until you have explicit approval to change them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just submitted an RFC. I'll note that I couldn't find any explicit statement about this in the python coding standards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did add a note about the conditions under which the code would be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've now wrapped this code in if useOldStarSelector: with the note above it describing when it will be removed.

# sort out the old/new star selector differences.

# configSel = StarSelectorConfig()
Expand All @@ -148,7 +170,15 @@ def _build_ccdImage(self, dataRef, associations, jointcalControl):
# print("new, old, shared:", len(stars1), len(stars2), len(aa))
# # import ipdb; ipdb.set_trace()

# TODO: NOTE: End of old source selector debugging block.
# print("%d stars selected in visit %d - ccd %d"%(len(stars2),
# dataRef.dataId["visit"],
# dataRef.dataId["ccd"]))
# associations.AddImage(stars2, tanwcs, md, bbox, filt, calib,
# dataRef.dataId['visit'], dataRef.dataId['ccd'],
# dataRef.getButler().get("camera").getName(),
# jointcalControl)
# TODO: End of old source selector debugging block.
# ------------------

if len(goodSrc.sourceCat) == 0:
print("no stars selected in ", dataRef.dataId["visit"], dataRef.dataId["ccd"])
Expand All @@ -161,13 +191,16 @@ def _build_ccdImage(self, dataRef, associations, jointcalControl):
dataRef.dataId['visit'], dataRef.dataId['ccd'],
dataRef.getButler().get("camera").getName(),
jointcalControl)
return tanwcs
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a blank return above (under if len(goodSrc.sourceCat) == 0), contrary to this one and the docstring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!


@pipeBase.timeMethod
def run(self, dataRefs):
"""
!Jointly calibrate the astrometry and photometry across a set of images.

@param dataRefs list of data references.
@param dataRefs list of data references.

@return (pipe.base.Struct) dataRefs that were fit (with updated WCSs) and old WCSs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should state the names of each element of the Struct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Done.

"""
if len(dataRefs) == 0:
raise ValueError('Need a list of data references!')
Expand All @@ -189,8 +222,9 @@ def run(self, dataRefs):
prof = pstats.Stats('jointcal_load_catalog.prof')
prof.strip_dirs().sort_stats('cumtime').print_stats(20)
else:
old_wcss = []
Copy link
Contributor

Choose a reason for hiding this comment

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

The name old_wcss is naughty. First, it's breaking naming rules (camelCase has been adopted in this file, so shouldn't be using under_scores). Second, it's breaking English rules (should be -es instead of -s).
I suggest oldWcsList.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair points. I've updated it to oldWcsList, including in the return struct.

for dataRef in dataRefs:
self._build_ccdImage(dataRef, associations, jointcalControl)
old_wcss.append(self._build_ccdImage(dataRef, associations, jointcalControl))
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a listcomp (clearer, often shorter, more pythonic):

old_wcss = [self._build_ccdImage(dataRef, associations, jointcalControl) for dataRef in dataRefs]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... I added the return of the old WCSes as a convenience for testing (and in case they get altered by future versions of jointcal). We may want to eventually not save the old WCSes, but the list comprehension line isn't too long, so I'll go with it.


matchCut = 3.0
# TODO: this should not print "trying to invert a singular transformation:"
Expand Down Expand Up @@ -299,6 +333,8 @@ def run(self, dataRefs):
self.log.warn('Failed to write updated Wcs: ' + str(e))
break

return pipeBase.Struct(dataRefs=dataRefs, old_wcss=old_wcss)


# TODO: Leaving StarSelector[Config] here for reference.
# TODO: We can remove them once we're happy with astrometryStarSelector.
Expand Down