DM-31964: Fixup dimensions on ProcessCcdWithFakesTask/MatchFakesTask #611
Conversation
morriscb
changed the title
python/lsst/pipe/tasks/matchFakes.py
Outdated
| for fakeCatRef in fakeCats: | ||
| cat = fakeCatRef.get( | ||
| datasetType=self.config.connections.fakeCats, | ||
| immediate=True) |
There was a problem hiding this comment.
There is no immediate keyword for get(). Or maybe I'm misunderstanding. DeferredDatasetHandle.get() doesn't take a datasetType parameter either.
python/lsst/pipe/tasks/matchFakes.py
Outdated
| # Make sure all data is within the inner part of the tract. | ||
| outputCat.append(cat[ | ||
| skyMap.findTractIdArray(cat[self.config.raColName], | ||
| cat[self.config.decColName], |
There was a problem hiding this comment.
@morriscb shouldn't these column names be self.config.ra_col and self.config.dec_col given that self.config.raColName and self.config.decColName are going to be deprecated?
| fakeCats = cT.Input( | ||
| doc="Set of catalogs of fake sources to draw inputs from. We will " | ||
| "from one of these that fully contains our exposure or the is " | ||
| "closest the visit boresight.", |
There was a problem hiding this comment.
@morriscb I guess the description here has a word missing between L68 and L69. L69 has the is. I believe the description also does not reflect the fact that it gathers fake sources from other neighboring tracts as done in composeFakeCat.
There was a problem hiding this comment.
As above, the doc string here doesn't read well - can you rephrase?
| outputCat.append(cat[ | ||
| skyMap.findTractIdArray(cat[self.config.insertFakes.raColName], | ||
| cat[self.config.insertFakes.decColName], | ||
| degrees=False) |
There was a problem hiding this comment.
@morriscb Same comment as above: self.config.insertfakes.raColName and self.config.insertFakes.decColName should be changed to self.config.ra_col and self.config.dec_col to remove deprecated column names.
| if externalSkyWcsCatalogRef.dataId["tract"] == tractId: | ||
| externalSkyWcsCatalog = externalSkyWcsCatalogRef.get( | ||
| datasetType=self.config.connections.externalSkyWcsTractCatalog) | ||
| break |
There was a problem hiding this comment.
@morriscb I have seen this return in some places with no externalSkyWcsCatalog because the condition is never satisfied. You could let it use the last externalSkyWcsCatalog with an else statement like:
else:
externalSkyWcsCatalog = externalSkyWcsCatalogRef.get(datasetType=self.config.connections.externalSkyWcsTractCatalog)
Similar problem for the photoCalib below in addition to a bug (see below L391).
There was a problem hiding this comment.
+1 for @surhudm's thinking above - it would be good for this large if/elif block to have a fall-back option in place.
There was a problem hiding this comment.
If the condition is never satisfied, that means there is no wcs or photocalib for that tract. I'm not sure what the safest fall back position. Likely people doing who have written the analysis code and use these calibs should decide.
| for externalPhotoCalibCatalogRef in externalPhotoCalibCatalogList: | ||
| if externalPhotoCalibCatalogRef.dataId["tract"] == tractId: | ||
| externalPhotoCalibCatalog = externalPhotoCalibCatalogRef.get( | ||
| datasetType=self.config.connections.externalSkyWcsTractCatalog) |
There was a problem hiding this comment.
+1 again - seems like a copy/paste typo.
morriscb
force-pushed
the
tickets/DM-31964
branch
from
709c5a2
to
1e9f491
Compare
|
|
||
| Returns | ||
| ------- | ||
| fakeCat : `pandas.core.frame.DataFrame` | ||
| fakeCats : `pandas.core.frame.DataFrame` |
There was a problem hiding this comment.
Please explain why this has been pluralized in the description for the parameter.
tests/test_matchFakes.py
Outdated
| @@ -197,7 +124,8 @@ def testTrimCat(self): | |||
| """Test that the correct number of sources are in the ccd area. | |||
| """ | |||
| matchTask = MatchFakesTask() | |||
| result = matchTask._trimFakeCat(self.fakeCat, self.exposure) | |||
| result = matchTask._trimFakeCat( | |||
| self.fakeCat, self.exposure) | |||
There was a problem hiding this comment.
This doesn't need to be on a new line.
Add code to concatenate fake catalogs. Fix linting. Add compose fakes to variablity code. Debug matchFakes. Add docs Simplify unittests. Remove unnessecary runQuantum test. Change coding dimensions. Fix linting. Output tract numbers. Fix linting. Access tractId Access tractId Fix ra/decColName usage. Remove immediate arg.
morriscb
force-pushed
the
tickets/DM-31964
branch
from
1e9f491
to
e1a9c35
Compare
Add code to concatenate fake catalogs.
Fix linting.
Add compose fakes to variablity code.
Debug matchFakes.
Add docs
Simplify unittests.
Remove unnessecary runQuantum test.
Change coding dimensions.
Fix linting.
Output tract numbers.
Fix linting.
Access tractId
Access tractId
Fix ra/decColName usage.