tickets/DM-22137 #16

fred3m · 2020-01-13T16:54:37Z

Integrate scarlet 1.0 into the stack and test on HSC data.

@pmelchior

In a conversation with @pmelchior it was suggested that normalizing the morphology might not be necessary with the new ADAM algorithm in scarlet, since it's better at breaking up degeneracies. In order to test this on HSC data this commit added functionality to rmove the normalization constraint. The result of the test was that deblending takes longer with poorer results when not using normalization, so this code will be stripped out in the next commit but is stored here for historical purposes.

This reverts commit 2122015.

Testing has shown that using the lower convergence threshold for scarlet 1.0 improves the residuals but takes more iterations. Execution on a few small 1k x 1k patches running for up to 1000 iterations showed that most models converge before 100 iterations but a significant fraction of blends don't converge until between 200-300 iterations, while blends that haven't converged by 300 iterations do not appear to ever converge. So we choose 300 iterations as the new cutoff.

This updates the unit tests to use the new API. This includes removing a few tests for functionality that has either been stripped or significantly modified.

taranu

Mostly fine but I left a few comments that should be addressed.

taranu · 2020-01-14T20:20:33Z

python/lsst/meas/extensions/scarlet/deblend.py

-        for center in footprint.peaks
-    ]
+    psfShape = (config.modelPsfSize, config.modelPsfSize)
+    model_psf = PSF(partial(gaussian, sigma=.8), shape=(None,)+psfShape)


Isn't that sigma=.8 important enough to set in the config with that default?

taranu · 2020-01-14T20:50:58Z

python/lsst/meas/extensions/scarlet/deblend.py

    usePsfConvolution = pexConfig.Field(
        dtype=bool, default=True,
        doc=("Whether or not to convolve the morphology with the"
             "PSF in each band or use the same morphology in all bands"))
+    modelPsfSize = pexConfig.Field(
+        dtype=int, default=11,
+        doc="The model PSF will be (modelPsfSize, modelPsfSize)")


"Model PSF side length in pixels" or something like that?

taranu · 2020-01-14T20:56:50Z

python/lsst/meas/extensions/scarlet/deblend.py

+                                                            "of the source converged, otherwise it"
+                                                            "contains the bit flag for parameters that"
+                                                            "failed, which might differ depending on the"
+                                                            "source type")


I find it somewhat confusing that deblend_blendConvergenceFailed is a flag whose name doesn't end in _flag (why not deblend_blendConvergence_flag?), whereas deblend_sourceConvergenceFlag isn't actually a flag (how about something like deblend_sourceConvergence_bits?).

deblend_blendConvergenceFailed -> deblend_blendConvergenceFailedFlag
deblend_sourceConvergenceFlag -> deblend_sourceConvergenceBitFlag

taranu · 2020-01-14T21:37:52Z

python/lsst/meas/extensions/scarlet/source.py

+    it finds a model that can be initialized. If all of the
+    models fail, including a `PointSource` model, then skip
+    the source.
+


I realize one can and should refer to scarlet's source here but it would be helpful to give a brief hint/summary as to how/why source initialization might fail.

clarified the docs

taranu · 2020-01-14T21:40:33Z

python/lsst/meas/extensions/scarlet/source.py

+                                          monotonic=monotonic, thresh=thresh)
+            if (np.any([np.isnan(c.sed) for c in components]) or
+                    np.all([c.sed <= 0 for c in source.components])):
+                logger.warning("Could not initialize")


This warning isn't very helpful. Why isn't this caught upstream?

This was part of a discussion that went on for weeks. scarlet used to throw a SourceInitError to inform the user that initialization failed but Peter felt strongly about having users check for a bad initialization and I finally got tired of fighting him on it.

taranu · 2020-01-14T22:24:36Z

python/lsst/meas/extensions/scarlet/source.py

+
+def morphToHeavy(source, peakSchema, xy0=Point2I()):
+    """Convert the morphology to a `HeavyFootprint`
+    """


Where are the Parameter docs? (2)

taranu · 2020-01-14T22:24:48Z

python/lsst/meas/extensions/scarlet/source.py

-    default initialization and update constraints for general sources in
-    LSST images.
+def checkConvergence(source):
+    """Check that a source converged
    """


Where are the Parameter docs? (1)

taranu · 2020-01-14T22:27:17Z

python/lsst/meas/extensions/scarlet/source.py

+
+
+def modelToHeavy(source, filters, xy0=Point2I(), observation=None, dtype=np.float32):
+    """Convert the model to a `MultibandFootprint`


Where are the Parameter docs? (3)

taranu · 2020-01-14T22:37:13Z

python/lsst/meas/extensions/scarlet/deblend.py

+
+    Parameters
+    ----------
+    mExposure: `MultibandExposure`


See https://developer.lsst.io/python/numpydoc.html#py-docstring-parameters

Must have a space before and after the colon

Parameter types should have the full namespace (lsst.foo.bar); I haven't checked to see if they work without them

It would be nice to maintain consistency (always end with a period; only add hyphens for lists)

taranu · 2020-01-14T22:52:44Z

python/lsst/meas/extensions/scarlet/source.py

+        Record for a peak in the parent `PeakCatalog`
+    observation: `LsstObservation`
+        The images, psfs, etc, of the observed data.
+    bbox: `Rect`


I don't think you imported Rect anywhere so this probably won't work.

It should, because I don't think this actually creates a link to the class. Otherwise I'd have to import Rect but never use it, which would cause flake8 to fail.

I thought the whole point was to create links, but apparently it doesn't? I'll have to ask about that.

Anyway, in this case since you're not actually importing Rect, the full namespace would be more useful.

Ok. It's a good thing you had me do this, I forgot that the stack doesn't call it Rect, it's Box2I.

taranu · 2020-01-16T18:47:40Z

python/lsst/meas/extensions/scarlet/__init__.py

@@ -24,4 +24,4 @@
 from .observation import *
 from .source import *
 from .blend import *
-from .deblend import *
+from . import deblend


Missing from .version import * (the docs won't build without it)

taranu · 2020-01-16T18:58:11Z

python/lsst/meas/extensions/scarlet/source.py

+        Record for a peak in the parent `PeakCatalog`
+    observation: `LsstObservation`
+        The images, psfs, etc, of the observed data.
+    bbox: `Rect`


I thought the whole point was to create links, but apparently it doesn't? I'll have to ask about that.

Anyway, in this case since you're not actually importing Rect, the full namespace would be more useful.

taranu · 2020-01-16T18:59:46Z

python/lsst/meas/extensions/scarlet/source.py

+
+    Parameters
+    ----------
+    frame: `LsstFrame`


Need a space before and after the colon here and in most of the added parameter docstrings.

Sadly I haven't been able to convince PyCharm to do this automatically.

This is an odd style guide requirement, as DM typically doesn't adhere to the spaces around operators rule. Numpydocs doesn't require this to compile properly, so this is just a waste of time (not your fault, I'm just venting). But I fixed it throughout the package, since I never use a space on both sides of the colon (because why would a sane person!).

Testing has shown that this threshold is too high and results in truncated boxes around extended objects. We lower the threshold from 5 to 1 to allow sources to grow larger.

fred3m · 2020-01-16T21:23:18Z

Should be ready to go now.

fred3m added 5 commits January 8, 2020 14:29

Implement ADAM (scarlet 1.0)

4edd1b9

Revert "Allow the normalization to be modified"

83e1010

This reverts commit 2122015.

Update tests

7fa8da7

This updates the unit tests to use the new API. This includes removing a few tests for functionality that has either been stripped or significantly modified.

taranu requested changes Jan 14, 2020

View reviewed changes

fred3m added 2 commits January 15, 2020 07:23

Update/add docstrings

cbd526a

Fix flake8 errors

6e5d690

taranu reviewed Jan 16, 2020

View reviewed changes

fred3m added 2 commits January 16, 2020 14:38

Lower detection threshold

7b59e42

Testing has shown that this threshold is too high and results in truncated boxes around extended objects. We lower the threshold from 5 to 1 to allow sources to grow larger.

Uddate docstrings

8ad4279

fred3m merged commit 74da54f into master Jan 17, 2020

fred3m deleted the tickets/DM-22137 branch January 17, 2020 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tickets/DM-22137 #16

tickets/DM-22137 #16

fred3m commented Jan 13, 2020

taranu left a comment

taranu Jan 14, 2020

taranu Jan 14, 2020

taranu Jan 14, 2020

fred3m Jan 15, 2020

taranu Jan 14, 2020

fred3m Jan 15, 2020

taranu Jan 14, 2020

fred3m Jan 15, 2020 •

edited

taranu Jan 14, 2020

taranu Jan 14, 2020

taranu Jan 14, 2020

taranu Jan 14, 2020

taranu Jan 14, 2020

fred3m Jan 15, 2020 •

edited

taranu Jan 16, 2020

fred3m Jan 16, 2020

taranu Jan 16, 2020

taranu Jan 16, 2020

taranu Jan 16, 2020

fred3m Jan 16, 2020

fred3m commented Jan 16, 2020



		def modelToHeavy(source, filters, xy0=Point2I(), observation=None, dtype=np.float32):
		"""Convert the model to a `MultibandFootprint`

tickets/DM-22137 #16

tickets/DM-22137 #16

Conversation

fred3m commented Jan 13, 2020

taranu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fred3m Jan 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fred3m Jan 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fred3m commented Jan 16, 2020

fred3m Jan 15, 2020 •

edited

fred3m Jan 15, 2020 •

edited