Tickets/dm 8145 #61
Tickets/dm 8145 #61
Conversation
There was a problem hiding this comment.
My only significant concern after reading through everything is the kernel filtering hack you include twice (e.g. line 401 of zogy.py). I suspect there will be use cases that will want some form of kernel filtering, so I recommend you write a function to replace the existing code.
tests/testZogy.py
Outdated
| import unittest | ||
|
|
||
| from builtins import range | ||
| from past.builtins import basestring |
There was a problem hiding this comment.
I believe we are supposed to avoid past.builtins at all costs. Also, it doesn't look like range or basestring is actually used anywhere, so they should be removed.
tests/testZogy.py
Outdated
| #import lsst.afw.geom as afwGeom | ||
| import lsst.afw.math as afwMath | ||
| #import lsst.meas.algorithms as measAlg | ||
| import lsst.daf.base as dafBase |
There was a problem hiding this comment.
dafBase appears to not be used anywhere, so should be removed. Also remove the commented-out imports.
tests/testZogy.py
Outdated
| del self.im2ex | ||
|
|
||
| def _compareExposures(self, D_F, D_R, Scorr=False, tol=0.02): | ||
| """Tests to compare the two images (diffim's or Scorr's). See below. |
There was a problem hiding this comment.
My flake8 would like a blank line between the summary line and the description. Here, and for many other docstrings.
python/lsst/ip/diffim/zogy.py
Outdated
|
|
||
| from .imageMapReduce import (ImageMapReduceConfig, ImageMapperSubtask, ImageMapperSubtaskConfig) | ||
|
|
||
| __all__ = ("ZogyTask", "ZogyConfig", |
There was a problem hiding this comment.
__all__ should be a list, not a tuple. Using a tuple can lead to some very odd errors in some cases.
python/lsst/ip/diffim/zogy.py
Outdated
| import lsst.pipe.base as pipeBase | ||
| import lsst.log | ||
|
|
||
| from .imageMapReduce import (ImageMapReduceConfig, ImageMapperSubtask, ImageMapperSubtaskConfig) |
There was a problem hiding this comment.
Do these imports need to be inside a tuple?
python/lsst/ip/diffim/zogy.py
Outdated
| return pipeBase.Struct(subExposure=subExposure) | ||
|
|
||
| psf1b = psf2b = None | ||
| if True and psf1.shape[0] == 41: # it's a measured psf (hack!) Note this *really* helps for measured psfs. |
There was a problem hiding this comment.
Since you're using this code again, it really should be a separate kernelFilter function. In that function, the size of the zero-padding should be configurable, but could have a default of 10 as you use here.
I don't think testing against the size of the psf equalling 41 is safe. Measured psfs will have a wide range of sizes.
| @@ -165,7 +165,10 @@ class ImageReducerSubtaskConfig(pexConfig.Config): | |||
| "sum": """add pixels from overlaps (probably never wanted; used for testing) | |||
| into correct location in new exposure""", | |||
| "average": """same as copy, but also average pixels from overlapped regions | |||
| (NaNs ignored)""" | |||
| (NaNs ignored)""", | |||
There was a problem hiding this comment.
On line 31 above, afwMath is imported but not used.
tests/testImageDecorrelation.py
Outdated
| @@ -115,19 +115,19 @@ def makeFakeImages(xim=None, yim=None, svar=0.04, tvar=0.04, psf1=3.3, psf2=2.2, | |||
| print(np.sqrt(psf1[0]**2 - psf2[0]**2)) | |||
| print('Offset:', offset) | |||
|
|
|||
| xim = np.arange(-256, 256, 1) if xim is None else xim | |||
| xim = np.arange(-128, 128, 1) if xim is None else xim | |||
There was a problem hiding this comment.
Please define a variable to store the value 128, and reference that here (and line 195)
tests/testZogy.py
Outdated
| import lsst.daf.base as dafBase | ||
| import testImageDecorrelation as testID # for makeFakeImages | ||
|
|
||
| from lsst.ip.diffim.zogy import (ZogyTask, ZogyConfig, ZogyMapReduceConfig) |
There was a problem hiding this comment.
Does this import need to be a tuple?
tests/testZogy.py
Outdated
| n_sources=10, psf_yvary_factor=varyPsf, | ||
| seed=666, verbose=False) | ||
| # Create an array corresponding to the "expected" subtraction (noise only) | ||
| np.random.seed(666) |
There was a problem hiding this comment.
Since you're setting this seed and the keyword seed on line 74 above to the same value, you should define seed = 666 as a variable and reference that in both places.
Bug fixes; return an Exposure D, S calc in Fourier/Real space all working Refactor astrometric variance calc Add debug features to imageMapReduce Zogy/ImageMapperSubtask working Refactor some params into config Add docstrings Initial start of zogy unit tests Small bug fixes to image-space found due to tests Completed all unit tests Add kwargs to propagate varAst params Make test images smaller; noise reproduceable Simplify simulated image size parameterization Updates; improve fakeImage func; remove nan/inf before FFT Update Kr,Kn kernel filtering
No description provided.