DM-31226: Add WCSFit task #1
Conversation
cmsaunders
force-pushed
the
tickets/DM-31226
branch
4 times, most recently
from
da8b329
to
3e582d8
Compare
cmsaunders
force-pushed
the
tickets/DM-31226
branch
from
3e582d8
to
b84420b
Compare
cmsaunders
force-pushed
the
tickets/DM-31226
branch
from
082b399
to
02ce01f
Compare
There was a problem hiding this comment.
Since I started commenting on other things, I looked through wcsfit.py and have several requested changes.
You use a mix of camelCase and snake_case here: can you please choose one (we're moving new code to snake_case, post-RFC-623) and use it consistently? Your method names have both (and also mixes, like _load_refCat!).
There was a problem hiding this comment.
This looks very good to me, with some small comments, and you should resolve conversations that you think are good.
One meta-question that I think has been brought up before is the name of the task. WcsFitTask is somewhat generic, and doesn't make it clear from the name that this is based on the gbdes code. Have you considered putting this in the task name? (At the minimum the documentation overview should say in the opening paragraph that this uses gbdes to do the fitting, perhaps with a link to the relevant DES paper?). The reason I think the name is important is that we (well, myself at least) are publicly talking about moving to "using gbdes for astrometric fitting", but then there's nothing in the pipeline definition that makes that apparent.
| systematicError = pexConfig.Field( | ||
| dtype=float, | ||
| doc=("Systematic error padding added in quadrature for the science catalogs (marcsec). The default" | ||
| "value is equivalent to 0.02 pixels for HSC"), |
| modelComponents = pexConfig.ListField( | ||
| dtype=str, | ||
| doc=("List of mappings to apply to transform from pixels to sky, in order of their application." | ||
| "Supported options are 'INSTRUMENT/DEVICE' and 'EXPOSURE'"), |
| deviceModel = pexConfig.ListField( | ||
| dtype=str, | ||
| doc=("List of mappings to apply to transform from detector pixels to intermediate frame. Map names" | ||
| "should match the format 'BAND/DEVICE/<map name>'"), |
| exposureModel = pexConfig.ListField( | ||
| dtype=str, | ||
| doc=("List of mappings to apply to transform from intermediate frame to sky coordinates. Map names" | ||
| "should match the format 'EXPOSURE/<map name>'"), |
| # Check if all components of the device and exposure models are | ||
| # supported. | ||
| for component in self.deviceModel: | ||
| if not (('poly' in component.lower()) or ('identity' in component.lower())): |
There was a problem hiding this comment.
I'm going to be "that guy" who mentions that this file has a mix of single-quotes and double-quotes. Now is the time to pick one. (I personally like single quotes, but black prefers double quotes; I leave it up to you to choose which format for these files.)
There was a problem hiding this comment.
Ok, I kept double quotes for documentation, but standardized to single quotes otherwise.
| """ | ||
| self.log.info("Gathering instrument, exposure, and field info") | ||
| # Set up an instrument object | ||
| instrument = wcsfit.Instrument(instrumentName) |
There was a problem hiding this comment.
If instrument is optional, what happens if you call this with None?
There was a problem hiding this comment.
Addressed in docs.
|
|
||
| return sourceIndices, columns | ||
|
|
||
| def make_YAML(self, inputVisitSummary, inputFile=None): |
There was a problem hiding this comment.
I think this should be called make_yaml not make_YAML.
| WCS-fitting object. | ||
| inputCatalogRefs : `list` | ||
| List of DeferredDatasetHandles pointing to visit-level source. | ||
| tables |
There was a problem hiding this comment.
The period comes on the previous line, and tables is sitting sadly alone.
cmsaunders
force-pushed
the
tickets/DM-31226
branch
from
f65fafd
to
e3b7442
Compare
cmsaunders
dismissed
parejkoj’s stale review
Eli approved this, and John is not available remove the blocker to merging.
No description provided.