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-32430: Create DECam DRP Processing Pipeline #208

Merged
merged 1 commit into from Dec 9, 2021
Merged

Conversation

leeskelvin
Copy link
Contributor

No description provided.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

Every one of the config lines in these files needs a comment describing why it is different from the default, please!

# HSC needs a higher order polynomial to track the steepness of the optical
# distortions along the edge of the field. Emperically, this provides a
# measurably better fit than the default order=5.
config.astrometryVisitOrder = 7
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you shouldn't set this for DECam unless we actually test how much it's better.

# For the deep fields with hundreds of visits, outlier rejection may take many
# weeks of runtime. By stopping outlier rejection when it ceases to have a
# significant effect on the model, we can bring compute time down to a few days.
config.astrometryOutlierRelativeTolerance = 0.002
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets leave this alone until we run jointcal on enough visits where the runtime matters: jointcal on DECam should be significantly faster than on HSC (less stars/visit and less detectors).

('r', 'r'),
('i', 'i'),
('z', 'z'),
('y', 'y'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove all of the above filterMaps: redundant filtermaps like these are unnecessary and can result in schema errors.

@leeskelvin leeskelvin force-pushed the tickets/DM-32430 branch 2 times, most recently from 7dea1b7 to bb34265 Compare November 22, 2021 18:02
@leeskelvin
Copy link
Contributor Author

Every one of the config lines in these files needs a comment describing why it is different from the default, please!

Many are copied over from obs_subaru, with some adapted for DECam, for example, pixel scale. This seems like a good place to start, to begin testing this pipeline against results output in HSC processing.

@leeskelvin leeskelvin force-pushed the tickets/DM-32430 branch 7 times, most recently from 1f97abd to 6ee6a46 Compare November 29, 2021 18:50
Copy link

@taranu taranu left a comment

Choose a reason for hiding this comment

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

Some minor requests for clarification.

config.photoCal.photoCatName = "ps1_pv3_3pi_20170110"
colors = config.photoCal.match.referenceSelection.colorLimits
# The following two color limits are adopted from obs_subaru for the HSC SSP survey
Copy link

Choose a reason for hiding this comment

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

Are they still suitable for DECam?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These limits are taken from Song Huang's original PR: #206. Song is one of the group leading the Merian data reduction effort, so I adopt them as per his request.

config.photoCal.match.referenceSelection.magLimit.maximum = 22.0
config.photoCal.colorterms.load(os.path.join(obsConfigDir, 'colorterms.py'))

# Better astrometry matching
Copy link

Choose a reason for hiding this comment

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

This comment could be more informative.

Copy link
Contributor Author

@leeskelvin leeskelvin Dec 1, 2021

Choose a reason for hiding this comment

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

Edit: Comment changed to: Number of bright stars to use. Sets the max number of patterns that can be tested.

matchConfig.matcher.maxMatchDistArcSec = 2.0
matchConfig.sourceSelector.active.excludePixelFlags = False

# Set to match defaults curretnly used in HSC production runs (e.g. S15B)
Copy link

Choose a reason for hiding this comment

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

currently; also, has this not changed since S15B? I guess not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, this config change is set to mimic current HSC production runs.

@@ -0,0 +1,15 @@
# Extinction coefficients for DECam filters for conversion from E(B-V) to extinction, A_filter.
Copy link

Choose a reason for hiding this comment

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

I'll note that these are all slightly lower than the HSC values in faro (ignore u band, I made that number up) - are we fine with that? Is it reflecting an actual physical difference in the filter set (no pun intended)?

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, these are different filters, so I'd expect these values to be somewhat different to HSC (albeit, close).

@@ -0,0 +1,712 @@
funcs:
Copy link

Choose a reason for hiding this comment

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

I gather these are copy-pasted from HSC; is there any way to keep them in sync?

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 - ultimately, DM-28862 will be the single go-to for all policy files. Following that change, all surveys should be using the same policy files. However, in the short term, I've had to place these policy files inside obs_decam to allow for this ticket to function correctly.

Copy link
Contributor

@mrawls mrawls left a comment

Choose a reason for hiding this comment

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

Most of these changes are great, and a big thank you for adding color terms! Per our Slack chat, I would really like to ensure each config override line (a) ideally differs from the task default and (b) has some documented motivation for existing, even if that is as simple as "obs_subaru does this so obs_decam probably ought to also."

Comment on lines +3 to +5
# Average color terms for the DECam filters are calculated using the Pickles stellar spectra atlas.
# The following values are provided by Song Huang (Tsinghua University; dr.guangtou@gmail.com)
# Color terms for u, Y, N419 and N964 bands are not currently available.
Copy link
Contributor

Choose a reason for hiding this comment

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

You're my hero for effectively doing DM-19284 for me 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS - all thanks to Song Huang (formerly here at Princeton) who actually did the hard work on this in his original PR - I'm just implementing it for us on this ticket!

config/transformObjectCatalog.py Show resolved Hide resolved
Comment on lines -2 to -7
config.match.refObjLoader.filterMap['u'] = 'g'
config.match.refObjLoader.filterMap['Y'] = 'y'
config.match.refObjLoader.filterMap['N419'] = 'g'
config.match.refObjLoader.filterMap['N540'] = 'g'
config.match.refObjLoader.filterMap['N708'] = 'i'
config.match.refObjLoader.filterMap['N964'] = 'z'
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure these aren't needed anymore? It is probably moot due to Gen2 going away, but the mapping is for non-ugrizy filters that need some "close enough" lookup refcat lookup filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the filter definitions to the file config/filterMap.py, as in obs_subaru, which seemed to be the most sensible thing to do. Broad band filters were removed because they were causing schema errors (see this comment from John P above, and this Slack thread for my original report of this error).

config/apertures.py Show resolved Hide resolved
config/assembleCoadd.py Outdated Show resolved Hide resolved
pipelines/DRP.yaml Show resolved Hide resolved
pipelines/DRP.yaml Outdated Show resolved Hide resolved
pipelines/DRP.yaml Outdated Show resolved Hide resolved
pipelines/DRP.yaml Outdated Show resolved Hide resolved
pipelines/DRP.yaml Show resolved Hide resolved
@leeskelvin leeskelvin force-pushed the tickets/DM-32430 branch 2 times, most recently from 94597fb to 4f3fee0 Compare December 7, 2021 21:45
Copy link
Contributor

@mrawls mrawls left a comment

Choose a reason for hiding this comment

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

Very nearly set! I'd prefer to see some of the coaddBase (makeWarp) and assembleCoadd configs in DRP.yaml itself instead of as obs_decam defaults, since we make less fancy templates in AP. Otherwise I think this is good to go.

@@ -0,0 +1,10 @@
# Configs shared between makeWarp and assembleCoadd
Copy link
Contributor

Choose a reason for hiding this comment

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

The settings in this config file assumes warps and coadds are being built for DRP reasons and not for AP template reasons; I suggest setting these in DRP.yaml instead of here (presumably in the makeWarp task).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All configs in the files coaddBase.py, makeWarp.py, compareWarpAssembleCoadd.py and assemblyCoadd.py have been moved to config sections in the main DRP.yaml pipeline file. This requires some amount of config duplication, as several of these configs are shared between quanta. I ran before/after diffs on the config outputs to confirm that the configs are identical.

Comment on lines 9 to 12
config.assembleStaticSkyModel.doApplyExternalPhotoCalib = True
config.assembleStaticSkyModel.externalPhotoCalibName = 'jointcal'
config.assembleStaticSkyModel.doApplyExternalSkyWcs = True
config.doFilterMorphological = True
Copy link
Contributor

Choose a reason for hiding this comment

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

As in coaddBase.py, these configs are DRP specific and not always desired for AP template building. I'd prefer to have them set in DRP.yaml instead of for all DECam things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above - this file no longer exists.


# Always produce these output bands in the Parquet coadd tables, no matter
# what bands are in the input.
# config.outputBands = ["g", "r", "i", "z", "y"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is commented out on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot! Removed.

pipelines/DRP.yaml Outdated Show resolved Hide resolved
pipelines/DRP.yaml Outdated Show resolved Hide resolved
pipelines/DRP.yaml Show resolved Hide resolved
@@ -1,8 +1,7 @@
# This is not a good place for pipelines
# Temporary DECam Pipelines
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose I'll allow it 😜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot I made this edit! Seemed like a good idea... but sad to see the old message go!

@leeskelvin leeskelvin merged commit 46ac641 into main Dec 9, 2021
@leeskelvin leeskelvin deleted the tickets/DM-32430 branch December 9, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants