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-38703: Add missing bright star feature to subtractBrightStars #815

Merged
merged 2 commits into from Sep 16, 2023

Conversation

leeskelvin
Copy link
Contributor

No description provided.

Copy link
Contributor

@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.

Broadly speaking, the logic in these changes appears fine in terms of how the functions are split up, named and called (I'm assuming they've been tested elsewhere). However, I think they would benefit greatly from a few judiciously-placed comments to explain the logic and flow. I have a half-remembered idea of how this task was supposed to work from the last time Morgan talked about it but it's not quite enough for me to piece together exactly what's happening at each stage.

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/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
@leeskelvin leeskelvin force-pushed the tickets/DM-38703 branch 5 times, most recently from 002c926 to e3582c4 Compare July 20, 2023 23:55
@leeskelvin leeskelvin force-pushed the tickets/DM-38703 branch 6 times, most recently from 05c86e3 to b30a4a1 Compare July 28, 2023 21:10
Copy link
Contributor

@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 further suggestions/comments.

@bazkiaei for your next review, please have a look at https://developer.lsst.io/work/flow.html#using-github-pull-requests and especially note the recommendation to use emoji reactions for minor fixes that do not require discussion.

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 Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Show resolved Hide resolved
@leeskelvin leeskelvin force-pushed the tickets/DM-38703 branch 3 times, most recently from 63650a3 to bfa2a5f Compare September 8, 2023 13:23
Copy link
Contributor

@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.

A few more nitpicks.

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 Show resolved Hide resolved
python/lsst/pipe/tasks/processBrightStars.py Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/subtractBrightStars.py Show resolved Hide resolved
@leeskelvin leeskelvin force-pushed the tickets/DM-38703 branch 5 times, most recently from caa6f40 to 8d0a339 Compare September 15, 2023 15:32
@leeskelvin leeskelvin merged commit b957171 into main Sep 16, 2023
2 checks passed
@leeskelvin leeskelvin deleted the tickets/DM-38703 branch September 16, 2023 00:30
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

4 participants