DM-27388: Implement metric system for fakes in AP #65

morriscb · 2020-12-08T22:35:17Z

No description provided.

Fix linting. Debug variable name. Debug metric name. Fix metric name.

Debug unittest. Remove unittest. Debug unittest.

kfindeisen

As brought up on Slack, there's not a good solution for what you're trying to do (and it's a much bigger question than a ticket about one metric). My comments below are based on the assumption that you take the AM1, AM2, etc. approach for now; that will at least keep measurements from different magnitude bins from overwriting each other.

kfindeisen · 2020-12-08T22:43:03Z

python/lsst/ap/pipe/metrics.py

+                          "package": "ap_pipe",
+                          "metric": "apFakesCompleteness"}):


Re: names, there should be a corresponding metric defined in the verify_metrics package (they cannot have periods, because they're namespaced as ap_pipe.apFakesCompleteness). I think creating a Measurement for a nonexistent metric is supposed to fail, but that's always been flaky.

Oh, now I see what you're trying to do. The current system really doesn't support parametrized metrics, unless you have a standardized set of values (e.g., apFakesCompleteness20t25, apFakesCompleteness25t30). Support for tweaking numbers was sort-of discussed on #dm-squash in a different context, but then nothing got acted on, and I'm not sure the proposed solution would work in Gen 3.

I'm not quite getting where to set these names (This is in addition to your comment later about connections). Should I just remove the automated naming in this code and just rely on doing everything in the pipeline config? If so, what all do I have to set?

I meant here that you'd have to add the metrics (the final names, with magnitudes) to https://github.com/lsst/verify_metrics/blob/master/metrics/ap_pipe.yaml if you want them to work everywhere (I think SQuaSH only uploads metrics defined in those files). You can use << notation (see pipe_tasks.yaml for examples) to cut down on the boilerplate.

Doing everything in the pipeline config would work; I think you'd just have to set connections.metric plus the magnitude values for each case.

So for the metric definitions in verify_metrics, something like:

apFakesCompleteness20t25: &apFakesCompleteness description: ... unit: "" reference: url: https://confluence.lsstcorp.org/pages/viewpage.action?spaceKey=~ebellm&title=Alert+Production+Metrics tags: - ap_verify - assoc - CCD apFakesCompleteness25t30: <<: *apFakesCompleteness description: ...

Okay, I basically have another ticket for this set of work: DM-26593. This one is mostly just to make the metric and get it tested so I'll add adding this to that ticket.

python/lsst/ap/pipe/metrics.py

kfindeisen · 2020-12-08T23:06:09Z

python/lsst/ap/pipe/metrics.py

+
+class ApFakesCompletenessMetricConfig(
+        MetricTask.ConfigClass,
+        InsertFakesConfig,


Might be worth a comment why you need to inherit from InsertFakesConfig (I assume it's to grab column names?).

kfindeisen · 2020-12-08T23:10:24Z

python/lsst/ap/pipe/metrics.py

+        doc="Minimum magnitude the mag distribution. All magnitudes requested "
+            "are set to the same value.",


I don't understand this description. Are you saying you draw some magnitude between magMin and magMax, and then set all the fake sources to have that one magnitude?

kfindeisen · 2020-12-08T23:21:54Z

python/lsst/ap/pipe/metrics.py

+        (`lsst.verify.Name`, read-only).
+        """
+        return Name(package=self.connections.package,
+                    metric=f"{self.connections.metric}Mag{self.magMin}t{self.magMax}")


Hmm... currently, MetricTask defines its output dataset name as "metricvalue_{package}_{metric}", without reference to Config.metricName. It might be better to have ApFakesCompletenessMetricConnections.__init__ set its own metric field based on the config, as then everything else would follow automatically. But I don't know if that would work; Connections don't like being edited.

Another solution would be to have MetricTaskConnections.__init__ set its output based on Config.metricName... I'm sure that would work, but it would be hard for anyone to understand, let alone maintain, something that tangled.

kfindeisen · 2020-12-09T00:13:33Z

python/lsst/ap/pipe/metrics.py

+            metricName = \
+                f"{self.config.metricName}"
+            magnitudes = matchedFakes[f"{self.config.magVar}" % band]


Suggested change

metricName = \

f"{self.config.metricName}"

magnitudes = matchedFakes[f"{self.config.magVar}" % band]

metricName = self.config.metricName

magnitudes = matchedFakes[self.config.magVar % band]

kfindeisen · 2020-12-09T00:27:01Z

tests/test_metrics.py

+
+        self.band = 'g'
+        magCut = 25
+        magMask = (fakeCat[f"{fakesConfig.magVar}" % self.band] < magCut)


Suggested change

magMask = (fakeCat[f"{fakesConfig.magVar}" % self.band] < magCut)

magMask = (fakeCat[fakesConfig.magVar % self.band] < magCut)

kfindeisen · 2020-12-09T00:32:04Z

tests/test_metrics.py

+        self.fakeCat = fakeCat.assign(diaObjectId=ids,
+                                      filterName=["g"] * len(fakeCat),
+                                      diaSourceId=ids)


This is a bit surprising. Shouldn't the catalog returned by fakesTask already have a filter column?

The columns here are to mimic the results of matchFakes, not create fakes. The fakes have all magnitudes in all bands created. This is faking columns after processing. Added a comment.

kfindeisen · 2020-12-09T00:32:52Z

tests/test_metrics.py

+    def testValid(self):
+        """Test the run method.
+        """
+        metricComplete = self.makeTask(20, 30)


Isn't this equivalent to self.task?

kfindeisen · 2020-12-09T00:35:58Z

tests/test_metrics.py

+    def testValidEmpty(self):
+        """Test the run method with a valid but zero result.
+        """
+        metricComplete = self.makeTask(25, 30)


I suggest making this

Suggested change

metricComplete = self.makeTask(25, 30)

metricComplete = self.makeTask(self.magCut, self.magCut + 5)

(with appropriate changes to setUp()). That way the test will be valid if the setup changes later.

Change valid test to use self.task. Add comment. Simplify string names in test and code. Add error handling to runQuantum. Add/modify docs. Add connections init for metric config. Remove automated renaming. Fix linting.

Change order of operation in test. Remove places. Change to exclicit value. Add places to test. Change to 2 places in Almost Equal comparison.

morriscb added 4 commits December 7, 2020 18:15

Add fakes metric task.

a39c760

Add runQuantum retrieve band.

bce56f0

Fix linting. Debug variable name. Debug metric name. Fix metric name.

Add metric unittest.

e41dd74

Debug unittest. Remove unittest. Debug unittest.

Add doc strings.

5e296b1

kfindeisen reviewed Dec 9, 2020

View reviewed changes

morriscb force-pushed the tickets/DM-27388 branch from 6d45e28 to 0c94d9b Compare December 10, 2020 22:50

morriscb added 2 commits December 14, 2020 16:40

Make magCut a class variable.

8e5d496

Change valid test to use self.task. Add comment. Simplify string names in test and code. Add error handling to runQuantum. Add/modify docs. Add connections init for metric config. Remove automated renaming. Fix linting.

Change to AlmostEqual in testing for clang.

a511a22

Change order of operation in test. Remove places. Change to exclicit value. Add places to test. Change to 2 places in Almost Equal comparison.

morriscb force-pushed the tickets/DM-27388 branch from b5f124d to a511a22 Compare December 14, 2020 22:41

morriscb merged commit c197479 into master Dec 14, 2020

morriscb deleted the tickets/DM-27388 branch December 14, 2020 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-27388: Implement metric system for fakes in AP #65

DM-27388: Implement metric system for fakes in AP #65

morriscb commented Dec 8, 2020

kfindeisen left a comment

kfindeisen Dec 8, 2020

kfindeisen Dec 8, 2020 •

edited

morriscb Dec 9, 2020

kfindeisen Dec 9, 2020

kfindeisen Dec 9, 2020 •

edited

morriscb Dec 10, 2020

kfindeisen Dec 8, 2020

kfindeisen Dec 8, 2020

morriscb Dec 9, 2020

kfindeisen Dec 8, 2020

kfindeisen Dec 9, 2020

kfindeisen Dec 9, 2020

kfindeisen Dec 9, 2020

morriscb Dec 9, 2020 •

edited

kfindeisen Dec 9, 2020

kfindeisen Dec 9, 2020

		doc="Minimum magnitude the mag distribution. All magnitudes requested "
		"are set to the same value.",

	magMask = (fakeCat[f"{fakesConfig.magVar}" % self.band] < magCut)
	magMask = (fakeCat[fakesConfig.magVar % self.band] < magCut)

	metricComplete = self.makeTask(25, 30)
	metricComplete = self.makeTask(self.magCut, self.magCut + 5)

DM-27388: Implement metric system for fakes in AP #65

DM-27388: Implement metric system for fakes in AP #65

Conversation

morriscb commented Dec 8, 2020

kfindeisen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Dec 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Dec 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morriscb Dec 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Dec 8, 2020 •

edited

kfindeisen Dec 9, 2020 •

edited

morriscb Dec 9, 2020 •

edited