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.

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

Tickets/DM-19942: ForcedPhotCcdTask in PipelineTask mode needs to accept references from multiple patches #168

Merged
merged 3 commits into from Mar 2, 2021

Conversation

natelust
Copy link
Contributor

No description provided.

Copy link
Contributor

@czwa czwa left a comment

Choose a reason for hiding this comment

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

Mostly docstring/style and places where I don't quite understand differences between gen2 and gen3 behavior.

python/lsst/meas/base/forcedPhotCcd.py Outdated Show resolved Hide resolved
containedIds = {0} # zero as a parent ID means "this is a parent"
for record in refCat:
if expPolygon.contains(record.getCoord().getVector()) and record.getParent() in containedIds:
record.setFootprint(record.getFootprint().transform(refWcs, expWcs, expRegion))
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't check for area=0 footprints the way the gen2 path does. Is this due to differences between expPolygon.contains() and self.references.fetchInBox()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is resurrected from an old commit, but from what I researched, if there is no footprint (or its zero area) then there is no coord associated with it (as none can be measured. This will cause getCoord and getVector to create special NaN associated objects. One of these objects is never contained inside any sphere geom regions, therefore this record is skipped.

"""
dataRef.put(sources, self.dataPrefix + "forced_src", flags=lsst.afw.table.SOURCE_IO_NO_FOOTPRINTS)

def getSchemaCatalogs(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct that this is a behind-the-scenes method that pipeBase.Task requires?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is related to CmdLineTask (though some gen3 might defer to it, but probably should be updated to reflect the dynamic nature of config names etc.

catalog.getTable().setMetadata(self.measurement.algMetadata)
datasetType = self.dataPrefix + "forced_src"
return {datasetType: catalog}

Copy link
Contributor

Choose a reason for hiding this comment

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

The following methods are no longer documented in the superclass (which is now PipelineTask, CmdLineTask).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a method that is in CmdLineTask and will go away when get get off of gen2

python/lsst/meas/base/forcedPhotCoadd.py Show resolved Hide resolved
return {datasetType: catalog}

def _getConfigName(self):
# Documented in superclass
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer documented in the superclass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of CmdLineTask

@@ -56,7 +56,6 @@
from .forcedMeasurement import *
from .forcedPhotCcd import *
from .forcedPhotCoadd import *
from .forcedPhotImage import *
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is staying, it is just no longer used as the base class for ForcedPhotCcd and ForcedPhotCoadd, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that, it was supposed to go, but in one of the rebase ontos I think it got put back in the tree

"""
refWcs = self.references.getWcs(dataRef)
exposure = self.getExposure(dataRef)
if psfCache is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is PSF caching not possible in gen3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be, but it would be done in a different way to this, with some kind of config attribute or something. However, I feel that is outside the scope of this ticket, which was just to orthogonalize the two tasks and make the task work with overlapping patches.

python/lsst/meas/base/forcedPhotCoadd.py Outdated Show resolved Hide resolved
ForcedPhotCcd assumed each CCD only overlappend one patch, fix.
@natelust natelust merged commit b9875a0 into master Mar 2, 2021
@natelust natelust deleted the tickets/DM-19942 branch March 2, 2021 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants