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-38744: Add auto option for how main star centroid is picked #30

Merged
merged 3 commits into from
Apr 18, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 41 additions & 4 deletions python/lsst/atmospec/processStar.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,15 @@ class ProcessStarTaskConfig(pipeBase.PipelineTaskConfig,
dtype=str,
doc="Method to get target centroid. "
"SPECTRACTOR_FIT_TARGET_CENTROID internally.",
default="fit",
default="auto",
allowed={
# TODO: probably want an "auto" mode
# note that although this config option controls
# SPECTRACTOR_FIT_TARGET_CENTROID, it doesn't map there directly,
# because Spectractor only has the concepts of guess, fit and wcs,
# and it calls "exact" "guess" internally, so that's remapped.
"auto": "If the upstream astrometric fit succeeded, and therefore"
" the centroid is an exact one, use that as an ``exact`` value,"
" otherwise tell Spectractor to ``fit`` the centroid",
"exact": "Use a given input value as source of truth.",
"fit": "Fit a 2d Moffat model to the target.",
"WCS": "Use the target's catalog location and the image's wcs.",
Expand Down Expand Up @@ -711,15 +717,46 @@ def getNormalizedTargetName(self, target):
return converted
return target

def _getSpectractorTargetSetting(self, inputCentroid):
"""Calculate the value to set SPECTRACTOR_FIT_TARGET_CENTROID to.

Parameters
----------
inputCentroid : `???`
Copy link

Choose a reason for hiding this comment

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

dict with no further information is better than nothing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, yeah, that wasn't intentional, sorry 😄

The input centroid to the task.

Returns
-------
centroidMethod : `str`
The value to set SPECTRACTOR_FIT_TARGET_CENTROID to.
"""

# if mode is auto and the astrometry worked then it's an exact
# centroid, and otherwise we fit, as per docs on this option.
if self.config.targetCentroidMethod == 'auto':
if inputCentroid['astrometricMatch'] is True:
self.log.info("Auto centroid is using exact centroid for target from the astrometry")
return 'guess' # this means exact
else:
self.log.info("Auto centroid is using FIT in Spectractor to get the target centroid")
return 'fit' # this means exact

# this is just renaming the config parameter because guess sounds like
Copy link

Choose a reason for hiding this comment

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

This is confusing, but from this comment and those above, it seems that is just how it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it's very confusing, hence choosing to rename it for us. I can negotiate renaming with Jérémy, but given we rename basically all the spectractor parameters into task config options, changing some of the values too doesn't feel so bad. I'll have a chat with him and if he agrees we can take it out in future.

# an instruction, and really we're saying to take this as given.
if self.config.targetCentroidMethod == 'exact':
return 'guess'

# all other options fall through
return self.config.targetCentroidMethod

def run(self, *, inputExp, inputCentroid, dataIdDict):
if not isDispersedExp(inputExp):
raise RuntimeError(f"Exposure is not a dispersed image {dataIdDict}")
starNames = self.loadStarNames()

overrideDict = {
# normal config parameters
'SPECTRACTOR_FIT_TARGET_CENTROID': ('guess' if self.config.targetCentroidMethod == 'exact'
else self.config.targetCentroidMethod),
'SPECTRACTOR_FIT_TARGET_CENTROID': self._getSpectractorTargetSetting(inputCentroid),
'SPECTRACTOR_COMPUTE_ROTATION_ANGLE': self.config.rotationAngleMethod,
'SPECTRACTOR_DECONVOLUTION_PSF2D': self.config.doDeconvolveSpectrum,
'SPECTRACTOR_DECONVOLUTION_FFM': self.config.doFullForwardModelDeconvolution,
Expand Down
16 changes: 0 additions & 16 deletions python/lsst/atmospec/spectraction.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,14 +183,6 @@ def spectractorImageFromLsstExposure(self, exp, xpos, ypos, *, target_label='',
image : `spectractor.extractor.images.Image`
The image.
"""
# TODO: passing exact centroids seems to be causing a serious
# and non-obvious problem!
# this needs fixing for several reasons, mostly because if we have a
# known good centroid then we want to skip the refitting entirely
xpos = int(np.round(xpos))
ypos = int(np.round(ypos))
self.log.debug(f'DM value at centroid: {exp.image.array[ypos, xpos]}\n')

# make a blank image, with the filter/disperser set
image = Image(file_name='', target_label=target_label, disperser_label=disperser_label,
filter_label=filter_label)
Expand Down Expand Up @@ -378,14 +370,6 @@ def run(self, exp, xpos, ypos, target, outputRoot=None, plotting=True):

# Upstream loads config file here

# TODO: DM-38264:
# passing exact centroids seems to be causing a serious
# and non-obvious problem! this needs fixing for several reasons,
# mostly because if we have a known good centroid then we want to skip
# the refitting entirely
xpos = int(np.round(xpos))
ypos = int(np.round(ypos))

filter_label, disperser = getFilterAndDisperserFromExp(exp)
image = self.spectractorImageFromLsstExposure(exp, xpos, ypos, target_label=target,
disperser_label=disperser,
Expand Down