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-43077: Convert all tasks to use CalibrateImageTask outputs #157

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

parejkoj
Copy link
Contributor

No description provided.

@parejkoj parejkoj force-pushed the tickets/DM-43077 branch 2 times, most recently from 544a9fa to cbc6df9 Compare September 19, 2024 05:55
- transformSourceTable
- consolidateSourceTable
- updateVisitSummary
description: |
Visit-level tasks.
Allowed data query constraints: visit

writeRecalibratedSourceTable, transformSourceTable run per-detector
transformSourceTable run per-detector
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
transformSourceTable run per-detector
transformSourceTable runs per-detector

description: |
Visit-level tasks.
Allowed data query constraints: visit

writeRecalibratedSourceTable, transformSourceTable run per-detector
transformSourceTable run per-detector
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
transformSourceTable run per-detector
transformSourceTable runs per-detector

Copy link
Member

Choose a reason for hiding this comment

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

or include reprocessVisitImage.

description: |
Visit-level tasks.
Allowed data query constraints: visit

writeRecalibratedSourceTable, transformSourceTable run per-detector
transformSourceTable runs per-detector
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why reprocessVisitImage is not included here like many others below?

@@ -52,15 +50,14 @@ subsets:
step2d:
subset:
- finalizeCharacterization
- writeRecalibratedSourceTable
Copy link
Member

Choose a reason for hiding this comment

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

Is reprocessVisitImage not included here on purpose?

class: lsst.pipe.tasks.characterizeImage.CharacterizeImageTask
config:
doApCorr: false
doDeblend: false
Copy link
Member

Choose a reason for hiding this comment

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

Could you comment briefly on Jira about the lack of this option in calibrateImage? deblending seems to be always on but they have been turned off in characterizeImage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deblending in calibrateImage is generally quite fast, because the detection threshold is high, and only happens after psf sources are found, not before. In characterizeImage it wasn't strictly necessary (see DM-36958) since that task was only finding psf sources and objectSizeStarSelector doesn't care about blendedness. There's no "turn off this step" options in calibrateImage, because all of the steps are either necessary for the output, or so fast you don't notice them.

Copy link
Member

Choose a reason for hiding this comment

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

I was less concerned about the speed but a comment in the code said turning it on messes things up. That comment is 8 years old now, but DM-36958 seem to have gotten stuck in want of an RFC. That's what has left me confused here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The important thing here is that we never want to deblend during psf determination, and calibrateImage doesn't.

@TallJimbo TallJimbo force-pushed the tickets/DM-43077 branch 2 times, most recently from bf425d1 to 4cc9227 Compare October 9, 2024 21:03
parejkoj and others added 6 commits October 15, 2024 11:37
* characterizeImage, writeSourceTable/writePreSourceTable are no longer
necessary, as those steps are done inside calibrateImage.
* Remove unnecessary configuration of quickLook as calibrateImage is
already quite stripped-down.
* Remove unnecessary imSim config that is already in the obs_lsst
config override.
This does the work of writeRecalibratedSourceTable, and writes both an
afw and Arrow version of the table, plus writes the final PVI. It
should be run after all visit-level calibrations have been finished and
the final visit summary produced.
calibrateImage detects significantly fewer sources overall, so needs
less deblending.
ci_imsim doesn't have much of the visit-level processing (e.g. fgcmcal), but
we still have to run reprocessVisitImage to get the full source catalog, since
calibrateImage only produces a catalog of stars, with fewer measurements.
This then requires treating the stars catalog as a "preSourceTable" and adding
some other tasks that would normally only be run in a "full" pipeline.
This should be replaced by analyzeCalibrateImageMetadata when it is
available (DM-46684).
to maintain inheritance of pipelines, and not duplicate
definitions of tasks in the pipelines
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.

4 participants