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-24062: Add tasks to generate parquet Source Table #264

Merged
merged 3 commits into from Apr 8, 2020

Conversation

yalsayyad
Copy link
Contributor

No description provided.

Copy link
Contributor

@morriscb morriscb left a comment

Choose a reason for hiding this comment

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

Requesting changes, but mostly because we should likely chat about where to put the new plugins. If they are generally applicable beyond just HSC it might make sense to just make them defaults in the various measurement tasks.

@@ -55,4 +55,5 @@
config.deblend.maskLimits["NO_DATA"] = 0.25 # Ignore sources that are in the vignetted region
config.deblend.maxFootprintArea = 10000

config.measurement.plugins.names |= ["base_Jacobian", "base_FPPosition"]
config.measurement.plugins.names |= ["base_Jacobian", "base_FPPosition",
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think? Put this in the config or the default on the task? Right now the base_Locals are defaults in the tasks multiBand and imageDiference.

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 thread here hasn't really helped decide one way or another: https://lsstc.slack.com/archives/C2JPMCF5X/p1585871128255400 People took to questioning Yes or No instead of A or B.

Currently, obs_subaru is the only place a Source.yaml exists which is the immediate user of these columns. I will either (a) keep it in obs_subaru and add a "TO DO: remove on DM-XXXXX" where DM-XXXXX is the ticket to write a obs_lsst/imsim/Source.yaml. while HSC folks are the guinea pigs in the meantime or (b) move it to calibrateConfig.setDefaults now. Concern there is that when people start using the columns, they'll be impossible to remove later if we start calibrating catalogs differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morriscb What's your preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, you say HSC is the only user of this currently but part of my work in creating these was to get the SDM running in AP which is currently DECam data. Having these values that are kind of fundamental to producing calibrated data products with the SDM be per obs package seems like a bunch of duplicated code that we don't need. If you want to make this a new ticket that's fine but I would need them to be done sooner rather than later.

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'll move it to calibrate now on this ticket since you need it for DECam AP! Thanks for letting me know.

@yalsayyad yalsayyad merged commit bf1bd6f into master Apr 8, 2020
@timj timj deleted the tickets/DM-24062 branch February 25, 2021 17:43
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

2 participants