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

DM-26553: Create gen3 unittests and test CreateApFakes pipeline tasks on data. #409

Merged
merged 3 commits into from Sep 28, 2020

Conversation

morriscb
Copy link
Contributor

No description provided.

@@ -681,8 +680,7 @@ def cleanCat(self, fakeCat, starCheckVal):
will be added.
"""

rowsToKeep = (((fakeCat[self.config.bulgeHLR] != 0.0) & (fakeCat[self.config.diskHLR] != 0.0))
| (fakeCat[self.config.sourceType] == starCheckVal))
rowsToKeep = ((fakeCat[self.config.bulgeHLR] != 0.0) & (fakeCat[self.config.diskHLR] != 0.0))
Copy link
Contributor

Choose a reason for hiding this comment

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

You've lost a line here from the check.

Copy link
Contributor Author

@morriscb morriscb Sep 18, 2020

Choose a reason for hiding this comment

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

Huh, don't know when that happened. Have put it back.

Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

Even after looking at CreateRandomApFakesTask, I'm a bit unsure about the use of tract as one of the dimensions for ProcessCcdWithFakesTask. It looks like the Gen 2 version of ProcessCcdWithFakesTask operated mostly on the detector level, with tracts only used for auxiliary data.

Is this a deliberate change between the Gen 2 and Gen 3 behavior? If not, it will cause problems with other tasks that assume fakes_calexp and fakes_src are detector-level.

I don't want to sign off on this until I understand what's going on.

python/lsst/pipe/tasks/processCcdWithFakes.py Show resolved Hide resolved

if not config.useUpdatedCalibs:
self.prerequisiteInputs.remove("wcs")
self.prerequisiteInputs.remove("photoCalib")
Copy link
Member

Choose a reason for hiding this comment

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

Should be inputs, not prerequisiteInputs.

Comment on lines 53 to 57
fakeCat = cT.Input(
doc="Catalog of fake sources to draw inputs from.",
name="{CoaddName}Coadd_fakeSourceCat",
storageClass="Parquet",
storageClass="DataFrame",
dimensions=("tract", "skymap")
Copy link
Member

Choose a reason for hiding this comment

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

@sr525, just to make sure, is it ok for the Gen 3 code to only take a tract-level *Coadd_fakeSourceCat, and not a detector-level fakeSourceCat? That seems to be the only substantial difference between the Gen 2 and Gen 3 versions of the task at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so I get what you're asking now. The catalogs are not different in how they are stored really. Both will be at the tract level, they just have different fluxes for the visit data vs coadd data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not? The visit level catalogues are needed for the variable sources, where the current way of using them is to make a catalogue specific to each calexp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait so are you saying we are going to produce specific catalogs for each ccdVisit vs cutting out the sources from a tract level catalog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, this conversation doesn't seem to be continuing so I'm going to go ahead and merge the ticket.

Add dropped or statement from insertFakes.
Remove tract from fakes output in procCcdWithFakes.
Make jointcal connecdtions optional.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants