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-39141: Add doRequirePrimary to source selectors. #331
Conversation
This object can be used as a `lsst.pex.config.Config` for configuring | ||
the column names to check for "detect_isPrimary". | ||
""" | ||
primaryColName = pexConfig.Field(dtype=str, default="detect_isPrimary", doc="Name of primary column") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe specify "Name of primary flag column"?
"""Select sources that have the detect_isPrimary flag set. | ||
|
||
This object can be used as a `lsst.pex.config.Config` for configuring | ||
the column names to check for "detect_isPrimary". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might help to add a comment about what a primary source is, e.g. from https://community.lsst.org/t/new-use-of-isprimary-that-began-in-w-2021-12/4957
detect_isPrimary
True when:
- A source is located on the interior of a patch and tract (
detect_isPatchInner
&detect_isTractInner
) - A source is not a sky object (
~merge_peak_sky
for coadds or~sky_source
for single visits) - A source is either an isolated parent that is un-modeled or deblended from a parent with multiple children (
isDeblendedSource
)
Or some distillation thereof...
|
||
Parameters | ||
---------- | ||
lsst.afw.table.SourceCatalog` or `pandas.DataFrame` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The leading catalog : `
is missing here
---------- | ||
lsst.afw.table.SourceCatalog` or `pandas.DataFrame` | ||
or `astropy.table.Table` | ||
Catalog of sources to which the requirements will be applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Singular "requirement"?
@@ -533,13 +563,17 @@ class ScienceSourceSelectorConfig(pexConfig.Config): | |||
doIsolated = pexConfig.Field(dtype=bool, default=False, doc="Apply isolated limitation?") | |||
doRequireFiniteRaDec = pexConfig.Field(dtype=bool, default=False, | |||
doc="Apply finite sky coordinate check?") | |||
doRequirePrimary = pexConfig.Field(dtype=bool, default=False, | |||
doc="Apply source primary check?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc reads a bit funny...I might have flipped source <-> primary
, but I'm not actually convinced that's better. May be add "is" to primary?
return sourceCat.get(self.primaryKey) | ||
else: | ||
return np.ones(len(sourceCat), dtype=bool) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this, I'm wondering if the same should be added to ObjectSizeStarSelectorTask
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the best of my knowledge, that selector is never run on catalogs that have had sky sources added; it's only run at the earliest time when we're looking for stars to measure the PSF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was just thinking that, in principle, someone could, so it wouldn’t hurt…your call!
No description provided.