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-38472: Reformat Bright Star Subtraction Processing Tasks #779

Merged
merged 1 commit into from May 3, 2023

Conversation

leeskelvin
Copy link
Contributor

No description provided.

@leeskelvin leeskelvin force-pushed the tickets/DM-38472 branch 3 times, most recently from 40bab59 to 2dc20e8 Compare April 27, 2023 21:14
Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

LGTM!

python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
@leeskelvin leeskelvin changed the title DM-38472: Refactor Bright Star Subtraction Processing Tasks DM-38472: Reformat Bright Star Subtraction Processing Tasks Apr 28, 2023
@leeskelvin leeskelvin force-pushed the tickets/DM-38472 branch 2 times, most recently from 70b5f2e to 45360de Compare April 28, 2023 02:16
@leeskelvin leeskelvin force-pushed the tickets/DM-38472 branch 3 times, most recently from cd2d5ea to 16b3947 Compare April 28, 2023 15:23
from lsst.afw import math as afwMath
from lsst.afw.fits import Fits, readMetadata
from lsst.afw.image import ImageF, MaskedImageF, MaskX
from lsst.afw.math import StatisticsControl, statisticsStack, stringToStatisticsProperty
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why do you prefer importing the stuff you need rather than importing the library and using it from there? Is it simply to reduce the line length when they appear 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.

Reducing line length is certainly a consideration, yes. I find the code more readable later on if my imports do the heavy lifting above. I also like that the imports are explicit about what is being used in this package. However, I'm certainly not committed to one way or the other, and would consider changing back if needs be?

With that said, following our discussion earlier in the week, I'll leave this as-is for now. Thanks for the useful discussions!

python/lsst/pipe/tasks/extended_psf.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/extended_psf.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/extended_psf.py Show resolved Hide resolved
python/lsst/pipe/tasks/extended_psf.py Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Outdated Show resolved Hide resolved
@leeskelvin leeskelvin merged commit a57fb48 into main May 3, 2023
1 check passed
@leeskelvin leeskelvin deleted the tickets/DM-38472 branch May 3, 2023 19:28
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

3 participants