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.

Sign up for GitHub

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

Jump to bottom

DM-17426 #1

Open

michitaro wants to merge 6 commits into HyperSuprime-Cam:master from michitaro:tickets/DM-17426

+173 −48

michitaro commented Mar 13, 2019 •

edited

Loading

See https://jira.lsstcorp.org/browse/DM-17426?focusedCommentId=191295&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-191295


          DM-17098

7ce5656

Author

michitaro commented Mar 13, 2019 •

edited

Loading

Default values are not updated now. so constructSky.py should be run with the following options:

--config \
     largeScaleBackground.xSize=8192 \
     largeScaleBackground.ySize=8192 \

skyCorrection.py should by run with the following options:

--config \
    bgModel.xSize=8192 \
    bgModel.ySize=8192 \
    bgModel2.minFrac=0.5 \
    bgModel2.xSize=256 \
    bgModel2.ySize=256 \
    bgModel2.smoothScale=256

michitaro added 5 commits

April 1, 2019 18:17


          Implement MaskObjectsTask

b2ebc0a


          Use MaskObjectsTask in constructSky

832078c


          Use MaskObjectsTask in skyCorrection

773e3a2


          2D interpolation in FocalPlaneBackground

a078e08


          add an extra focalplane sky-subtraction

876d95d

michitaro force-pushed the tickets/DM-17426 branch from 81d60d4 to 876d95d Compare

April 1, 2019 09:34

PaulPrice requested changes

View reviewed changes

PaulPrice left a comment

This is a good start, but we need to make some changes in order to meet LSST's standards.

python/lsst/pipe/drivers/background.py

+                      self.detection.doTempLocalBackground = False
+                      self.detection.doTempWideBackground = False
+                      self.detection.thresholdValue = 2.5
+                      # self.detection.thresholdPolarity = "both"

PaulPrice Apr 2, 2019

Remove commented code.

python/lsst/pipe/drivers/background.py

+                  def validate(self):
+                      assert not self.detection.reEstimateBackground
+                      assert not self.detection.doTempLocalBackground
+                      assert not self.detection.doTempWideBackground

PaulPrice Apr 2, 2019

Can you change these into RuntimeErrors with useful error messages?

python/lsst/pipe/drivers/background.py

+                  ConfigClass = MaskObjectsConfig
+                  def __init__(self, *args, **kwargs):
+                      super(MaskObjectsTask, self).__init__(*args, **kwargs)

PaulPrice Apr 2, 2019

Python 3, so just need super().__init__(*args, **kwargs).

python/lsst/pipe/drivers/background.py



		class MaskObjectsTask(Task):
		"""MaskObjectsTask

PaulPrice Apr 2, 2019

Need a better short summary than the name of the task. Maybe "Iterative masking of objects on an image"?

python/lsst/pipe/drivers/background.py Show resolved Hide resolved

python/lsst/pipe/drivers/background.py

+                  @staticmethod
+                  def _normalDist(x, s=1., m=0.):
+                      ''' Normal Distribution '''
+                      return 1. / (s * numpy.sqrt(2. * numpy.pi)) * numpy.exp(-(x-m)**2/(2*s**2)) / (s * numpy.sqrt(2*numpy.pi))

PaulPrice Apr 2, 2019

Why two normalisation factors of (s * numpy.sqrt(2. * numpy.pi))?

python/lsst/pipe/drivers/background.py

@@ @@ -785,3 +794,52 @@ def run(self, exp): @@
                           detected = mask.array & mask.getPlaneBitMask(['DETECTED']) > 0
                           exp.maskedImage.image.array[detected] = smooth.getImage().getArray()[detected]
+              class NanSafeSmoothing:

PaulPrice Apr 2, 2019

I think this section needs a different implementation.
It looks like you're using class here just as a namespace, which feels dirty. I don't think it adds anything beyond making a stand-alone function with a couple of embedded functions.
A direct convolution implemented in python is going to be crazy slow (especially since you're not making use of the fact that the kernel is separable). Is there a reason you can't use the convolution functionality in afw, as is done in SourceDetectionTask? If you're worried about NaNs, you can always (temporarily?) replace them with zeros when you do the convolution.

Author

michitaro Apr 2, 2019 •

edited

Loading

If you're worried about NaNs, you can always (temporarily?) replace them with zeros when you do the convolution.

This is why I didn't use the existing convolution code. Replacing NaNs with 0 is not equivalent to my code.
Sometimes super-pixels on the edge of the field are bit high. In that case it is better that the super-pixels out of the field are as the are extrapolated (this is what I intended to) than assuming that they are zero (replacing NaNs with 0). In other words, replacing NaNs with 0 leads low sky estimation near the bright edge.

A direct convolution in Python is crazy slow, but the targets of this code are relatively small images such as FocalPlaneBackground._values whose dimensions are about 140x150. Convolutions on such images end in about 0.5 second on my mac.

It looks like you're using class here just as a namespace, which feels dirty. I don't think it adds anything beyond making a stand-alone function with a couple of embedded functions.

I totally agree with you. I will put these functions flat in the module.

PaulPrice Apr 3, 2019

I figured out the difference between your convolution and the vanilla convolution. This will allow us to condense the code, and probably run it faster:

def safeConvolve(array, sigma=2):
    bad = np.isnan(array)
    safe = np.where(bad, 0.0, array)
    convolved = gaussian_filter(safe, sigma, mode="constant", cval=0.0)
    corr = np.where(bad, 0.0, 1.0)
    ones = np.ones_like(array)
    numerator = gaussian_filter(ones, sigma, mode="constant", cval=0.0)
    denominator = gaussian_filter(corr, sigma, mode="constant", cval=0.0)
    return convolved*numerator/denominator

This reproduces your result:

Author

michitaro Apr 4, 2019

Excellent solution.

python/lsst/pipe/drivers/skyCorrection.py

                           # Persist camera-level image of calexp
                           image = makeCameraImage(camera, exposures)
                           expRef.put(image, "calexp_camera")
                           pool.mapToPrevious(self.write, dataIdList)
+                  def smoothFocalPlaneSubtraction(self, camera, pool, dataIdList):
+                      '''Do 2nd Focal Plane subtraction

PaulPrice Apr 2, 2019

Please use numpydoc style for docstrings.

python/lsst/pipe/drivers/skyCorrection.py

+                      for ii, bg in enumerate(bgModelList):
+                          self.log.info("Background %d: %d pixels", ii, bg._numbers.array.sum())
+                          bgModel.merge(bg)
+                      exposures = pool.mapToPrevious(self.subtractModel, dataIdList, bgModel)

PaulPrice Apr 2, 2019

This is the same code as before (lines 138-143). Can you factor it into a common method so the code isn't duplicated?

python/lsst/pipe/drivers/skyCorrection.py

+                      '''Do 2nd Focal Plane subtraction
+                      After doSky, we get smooth focal plane image.
+                      (Before doSky, sky pistons remain in HSC-G)

PaulPrice Apr 2, 2019

This comment is a bit too terse to understand what you mean.

michitaro commented

View reviewed changes

Author

michitaro left a comment

I would like your comment on this.

I've started revising the code for other comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet