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-23071: Reconfigure aperture correction source selector for bright stars #242

Merged
merged 2 commits into from Jan 17, 2020

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Jan 16, 2020

No description provided.

config.measureApCorr.sourceSelector['science'].signalToNoise.maximum = None
config.measureApCorr.sourceSelector['science'].signalToNoise.fluxField = 'base_PsfFlux_instFlux'
config.measureApCorr.sourceSelector['science'].signalToNoise.errField='base_PsfFlux_instFluxErr'
config.measureApCorr.sourceSelector.name = 'science'
Copy link
Contributor

@laurenam laurenam Jan 16, 2020

Choose a reason for hiding this comment

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

Looks like you need spaces around the equals sign in
config.measureApCorr.sourceSelector['science'].signalToNoise.errField='base_PsfFlux_instFluxErr'

It might also be nice to add a quick comment along the lines of:
# For aperture correction modeling, only use objects that were used in the PSF model and have S/N > 200

Also, VERY minor nitpick...most of (arghh...already internally inconsistent!) the rest of the file uses double quotes (and I was recently asked why pick one or the other...I tend to just always go double but try to be consistent with the rest of the file as is...not really possible here, so do as you wish!).

PS This is not an official review, I was just curious what S/N threshold you landed on. 👍 on 200 :)

Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

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

@laurenam makes some good points, and I don't have anything to add to them.

@erykoff erykoff merged commit c1fa266 into master Jan 17, 2020
@erykoff erykoff deleted the tickets/DM-23071 branch January 17, 2020 18:07
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