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-36408: Add Coadd PSF and multiband source fitting tasks #749

Merged
merged 2 commits into from Feb 16, 2023

Conversation

taranu
Copy link
Contributor

@taranu taranu commented Jan 19, 2023

These tasks operate on coadd images and are meant to be run sequentially, in contrast to the old monolithic fitting MultibandFitTask in fit_multiband.py, which did both PSF and source fitting in one task. Also, unlike the old task (which will be deprecated soon), the outputs are astropy tables rather than SourceCatalogs. The structure is otherwise similar, with the actual work done by a configurable subtask (not included).

Copy link
Contributor

@fred3m fred3m left a comment

Choose a reason for hiding this comment

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

Just a few comments and minor changes requested. Although there seems to be a lot of duplicate code between the different tasks, and I wonder if you could abstract more of it out?

python/lsst/pipe/tasks/fit_coadd_multiband.py Show resolved Hide resolved
python/lsst/pipe/tasks/fit_coadd_multiband.py Show resolved Hide resolved
python/lsst/pipe/tasks/fit_coadd_psf.py Show resolved Hide resolved
@taranu
Copy link
Contributor Author

taranu commented Feb 1, 2023

Just a few comments and minor changes requested. Although there seems to be a lot of duplicate code between the different tasks, and I wonder if you could abstract more of it out?

About the duplication - I accidentally left in a couple of unnecessary methods in the PSF fitting task. Other than the base CatalogExposure dataclass that I left in fit_multiband.py, I don't think there's anything beneficial to be shared with the multiband fitting task.

fit_coadd_multiband.py was indeed adapted from fit_multiband.py and some of the code is identical, but I plan to just axe everything but CatalogExposure in fit_multiband.py. I'm not sure if I need to formally deprecate it considering that the only two implementations of the subtasks are in lsst-dm repos, but I will if necessary.

@fred3m
Copy link
Contributor

fred3m commented Feb 1, 2023

No, I don't think that you need to deprecate lsst-dm code.

@taranu
Copy link
Contributor Author

taranu commented Feb 1, 2023

I meant deprecating the MultibandFitTask in fit_multiband.py, here in pipe_tasks, which is not part of any pipelines in part because there are no implementations of MultibandFitSub[Config/Task] in lsst, only in lsst-dm.

@fred3m
Copy link
Contributor

fred3m commented Feb 1, 2023

Right, sorry, that's what I meant too. Even though the task is in pipe_tasks, it requires code from lsst-dm, so I feel like it would be silly to deprecate (effectively) unused code.

@taranu taranu force-pushed the tickets/DM-36408 branch 2 times, most recently from 72d824f to 301e94a Compare February 9, 2023 20:17
This task is essentially an interface that was only ever implemented in
lsst-dm repos that are no longer in active use and likely bitrotted.
Furthermore, the design is a monolith that does PSF and source modelling,
which is a pattern we are trying to discourage.
@taranu taranu merged commit fe0d0b4 into main Feb 16, 2023
@taranu taranu deleted the tickets/DM-36408 branch February 16, 2023 22:05
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