DM-42458: Modify variance plane when injecting sources and optionally vary injected light #29

jjbuchanan · 2024-01-11T21:35:15Z

No description provided.

timj · 2024-01-11T21:43:16Z

@jjbuchanan can you check that the line endings on your files haven't been modified? The diff is showing entire changes of file content even in the headers which haven't changed.

jjbuchanan · 2024-01-11T22:33:02Z

@timj I've fixed the line endings, and the diffs look better now.

jjbuchanan · 2024-01-13T06:57:29Z

Fixed the whitespace and import order issues that were producing flake8 and isort errors.

jmeyers314

Looks good, James. I'd still like to run this through a few pipelines to make sure everything makes sense there, but thought I'd give you some initial feedback today.

In addition to the line-by-line stuff, I think you can probably add some unit tests. Things like comparing add_noise=True with add_noise=False with otherwise identical inputs.

jmeyers314 · 2024-01-22T21:51:19Z

python/lsst/source/injection/inject_coadd.py

@@ -72,6 +112,13 @@ class CoaddInjectTask(BaseInjectTask):
    _DefaultName = "coaddInjectTask"
    ConfigClass = CoaddInjectConfig

+    def run(self, injection_catalogs, input_exposure, psf, photo_calib, wcs):
+        self.log.info("Fitting flux vs. variance in each pixel.")
+        self.config.variance_scale = self.get_variance_scale(input_exposure)


I don't think we should pass calculated values through the config. The config gets written to the butler once per collection, so having values that differ (from exposure to exposure) within a collection will get lost.

Got it. I can instead pass variance_scale as an argument to BaseInjectTask.run(), on the same footing as things like photo_calib.

jmeyers314 · 2024-01-22T21:52:02Z

python/lsst/source/injection/inject_coadd.py

@@ -82,3 +129,124 @@ def runQuantum(self, butler_quantum_context, input_refs, output_refs):
        input_keys = ["injection_catalogs", "input_exposure", "sky_map", "psf", "photo_calib", "wcs"]
        outputs = self.run(**{key: value for (key, value) in inputs.items() if key in input_keys})
        butler_quantum_context.put(outputs, output_refs)
+
+    def get_variance_scale(self, exposure):


Should add a docstring here explaining the basic algorithmic idea. Why there are two rounds of RANSAC, for example.

I came up with one. The algorithm has multiple parts that need explaining so it's a little verbose, but hopefully okay.

jmeyers314 · 2024-01-22T21:53:28Z

python/lsst/source/injection/inject_coadd.py

@@ -82,3 +129,124 @@ def runQuantum(self, butler_quantum_context, input_refs, output_refs):
        input_keys = ["injection_catalogs", "input_exposure", "sky_map", "psf", "photo_calib", "wcs"]
        outputs = self.run(**{key: value for (key, value) in inputs.items() if key in input_keys})
        butler_quantum_context.put(outputs, output_refs)
+
+    def get_variance_scale(self, exposure):
+        x = exposure.image.array.flatten()


I think these variables are large enough in scope to get spelled out. Maybe flux and var or variance, but not x and y.

Also, I think .flatten will always produce a copy, where .ravel will only copy when necessary. So as long as these are read-only (I think that's right?) .ravel might be a tad faster.

Agree on the variable names.

Interesting point about flatten vs. ravel. I'll go with ravel, while noting that I'm also changing the way that nan pixels are handled.

jmeyers314 · 2024-01-22T21:54:13Z

python/lsst/source/injection/inject_coadd.py

+        bad_pixels = ~np.isfinite(x) | ~np.isfinite(y)
+        # Replace bad pixel values with the image median
+        if np.sum(bad_pixels) > 0:
+            median_image_value = np.median(x)


Probably needs to be np.nanmedian?

Instead of replacing nan pixels with the median value, I think it may be best to just ignore them, by assigning flux = flux[good_pixels].

jmeyers314 · 2024-01-22T21:56:01Z

python/lsst/source/injection/inject_coadd.py

+            kmeans.labels_ == np.argmax(label_counts)
+        ]
+        if len(stable_fit_seeds == 0):
+            # Throw a warning


Need to find an appropriate warning to raise.

Implemented, and another one at the very end if the final slope ends up being negative.

jmeyers314 · 2024-01-22T21:59:56Z

python/lsst/source/injection/inject_engine.py

+
+            # If the smooth image can be drawn successfully,
+            # we can do everything else.
+            if draw_succeeded:


Doesn't the try/except either succeed, continue (on GalSimFFTSizeError), or halt (on any other error)?
I think this is always true, so unneeded.

Good point.

jmeyers314 · 2024-01-22T22:00:59Z

python/lsst/source/injection/inject_engine.py

+                if add_noise:
+                    # For generating noise,
+                    # variance must be meaningful.
+                    if np.sum(variance_template.array < 0) > 0:


I'm guessing np.any is a hair faster. It has the possibility to short-circuit for example.

jmeyers314 · 2024-01-22T22:02:03Z

python/lsst/source/injection/inject_visit.py

@@ -104,6 +108,13 @@ class VisitInjectTask(BaseInjectTask):
    _DefaultName = "visitInjectTask"
    ConfigClass = VisitInjectConfig

+    def run(self, injection_catalogs, input_exposure, psf, photo_calib, wcs):
+        self.log.info("Fitting flux vs. variance in each pixel.")
+        self.config.variance_scale = self.get_variance_scale(input_exposure)


Same as earlier, I don't think we can pass calculated values through the config.

Changed as noted above.

jmeyers314 · 2024-01-22T22:02:18Z

python/lsst/source/injection/inject_visit.py

@@ -128,3 +139,35 @@ def runQuantum(self, butler_quantum_context, input_refs, output_refs):
        input_keys = ["injection_catalogs", "input_exposure", "sky_map", "psf", "photo_calib", "wcs"]
        outputs = self.run(**{key: value for (key, value) in inputs.items() if key in input_keys})
        butler_quantum_context.put(outputs, output_refs)
+
+    def get_variance_scale(self, exposure):


Needs a docstring.

wmwv · 2024-02-02T01:42:02Z

python/lsst/source/injection/inject_visit.py

+            y[bad_pixels] = median_variance_value
+
+        # Only fit pixels with at least this much inst flux
+        brightness_cutoff = 500


I might suggest that this threshold should be stored and thus accessible in the config for the Task.

Yes, will do.

wmwv · 2024-02-02T01:44:37Z

python/lsst/source/injection/inject_coadd.py

+            ransac = RANSACRegressor(
+                loss="squared_error",
+                residual_threshold=self.config.threshold_scale_1 * linear_mse,
+                max_trials=1000,


Should this hard-coded value for max_trials be accessible at the function keyword level, or even the Task config level?

It makes sense to put it at the function keyword level at least, and I'm fine with putting it in the Task config if people are happy with the proliferation of config options. Ideally this value should be high enough so that it's never the thing that causes a pathology in practice, and it seems like scikit-learn's default value is a little low, which is why I specify a different value here.

wmwv · 2024-02-02T01:44:59Z

python/lsst/source/injection/inject_coadd.py

+        # K-means cluster the first round of fits,
+        # to find the most stable results.
+        kmeans = KMeans(
+            n_clusters=self.config.n_clusters_1, random_state=self.config.kmeans_seed_1, n_init=10


Should n_init be accessible at the Task config level?

I think that makes sense. I can put it there.

Modify variance plane and optionally vary injected light

3b19729

Modify variance plane and optionally vary injected light

47d922d

jjbuchanan force-pushed the tickets/DM-42458 branch from 3b19729 to 47d922d Compare January 11, 2024 22:28

Fix whitespace and import order

140cb76

black formatting

8447361

jmeyers314 requested changes Jan 22, 2024

View reviewed changes

wmwv reviewed Feb 2, 2024

View reviewed changes

jjbuchanan changed the title ~~Modify variance plane when injecting sources and optionally vary injected light~~ DM-42458: Modify variance plane when injecting sources and optionally vary injected light Feb 14, 2024

Respond to code review comments

4d9478c

jmeyers314 merged commit f4d6588 into lsst:tickets/DM-42458 Mar 5, 2024
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-42458: Modify variance plane when injecting sources and optionally vary injected light #29

DM-42458: Modify variance plane when injecting sources and optionally vary injected light #29

jjbuchanan commented Jan 11, 2024

timj commented Jan 11, 2024

jjbuchanan commented Jan 11, 2024

jjbuchanan commented Jan 13, 2024 •

edited

jmeyers314 left a comment

jmeyers314 Jan 22, 2024

jjbuchanan Feb 13, 2024

jmeyers314 Jan 22, 2024

jjbuchanan Feb 14, 2024

jmeyers314 Jan 22, 2024

jjbuchanan Feb 14, 2024

jmeyers314 Jan 22, 2024

jjbuchanan Feb 14, 2024

jmeyers314 Jan 22, 2024

jjbuchanan Feb 14, 2024

jmeyers314 Jan 22, 2024

jjbuchanan Feb 14, 2024

jmeyers314 Jan 22, 2024

jjbuchanan Feb 14, 2024

jmeyers314 Jan 22, 2024

jjbuchanan Feb 14, 2024

jmeyers314 Jan 22, 2024

jjbuchanan Feb 14, 2024

wmwv Feb 2, 2024

jjbuchanan Feb 2, 2024

wmwv Feb 2, 2024 •

edited

jjbuchanan Feb 2, 2024

wmwv Feb 2, 2024

jjbuchanan Feb 2, 2024

DM-42458: Modify variance plane when injecting sources and optionally vary injected light #29

DM-42458: Modify variance plane when injecting sources and optionally vary injected light #29

Conversation

jjbuchanan commented Jan 11, 2024

timj commented Jan 11, 2024

jjbuchanan commented Jan 11, 2024

jjbuchanan commented Jan 13, 2024 • edited

jmeyers314 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

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

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

wmwv Feb 2, 2024 • 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

jjbuchanan commented Jan 13, 2024 •

edited

wmwv Feb 2, 2024 •

edited