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-48955: Add illumination correction creation pipeline and BPS template files #299

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

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Feb 18, 2025

No description provided.

# smaller catalog after it's integrated.
# Goal is to remove all of these overrides except the aperture radii,
# which we instead want to set to the same as the compensated tophat
# radii below for lsst.fgcmcal.fgcmOutputProducts.FgcmOutputProductsTask
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth it to try to do the copy of this TODO for this variance of calibrateImage now: I was being really careful to rock the boat as little as possible when switching to calibrateImage in DRP so I could separate structural changes from algorithmic changes as much as possible. But I was especially trying to make sure I didn't break any analysis tooling that was expecting a deeper src catalog than the default calibrateImage produces, and you don't have to worry about that here. And we do know the default calibrateImage configs work fine in terms of robustness, since that's basically what AP runs.

Copy link
Contributor Author

@erykoff erykoff Feb 19, 2025

Choose a reason for hiding this comment

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

I can't run the "plain" variant; I need the fgcmcal inputs. Though I can cut down the apertures. But I tried removing plugins and it crashed because the transformation schema expected all these outputs and at that point I didn't want to open that can of hornets.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yuck. It hadn't occurred to me before that doing the standardization before the consolidate step and doing the consolidate step before the association meant that we can't easily run variants of calibrateImage -> {fgcm, gbdes} without writing new functor files. It's going on my list.

(My list of things to start daydreaming about ripping out and wholly replacing, that is; it's not quite the same as your List.)

For now, maybe just change the TODO comment to say that we can't let this configuration diverge meaningfully from the DRP one until we add a new functor file for it (and then make a ticket to add a new functor file). I think that's actually moderately important, not because we care so much about running unnecessary algorithms here, but because having to sync this with the DRP configuration is problematic.

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 defined a new functor file and overrode the appropriate configs.

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.

2 participants