DM-35621: Add matched-to-reference table plots and metrics #17

taranu · 2022-07-22T19:13:35Z

No description provided.

natelust

I think this mostly looks good, but I have some concerns in places.

natelust · 2022-07-29T21:19:59Z

python/lsst/analysis/tools/actions/plot/scatterplotWithTwoHists.py


 class ScatterPlotStatsAction(KeyedDataAction):
    vectorKey = Field[str](doc="Vector on which to compute statistics")
-    highSNSelector = ConfigurableActionField[VectorAction](
+    highSNSelector = ConfigurableActionField[SnSelector](


if you do this, it means no other actions (that are not subclasses of SnSelector) could ever be assigned to this field, I am not sure we are ready to declare that that is an interface vs an implementation.

Hmm. I'm also a bit on the fence between worrying that leaving it as a VectorAction is a trap (and it being named SNSelector implies that it was intended as an interface), and acknowledging that someone may implement an alternative to SNSelector. But they could just change the typing in the latter case, no?

natelust · 2022-07-29T21:20:41Z

python/lsst/analysis/tools/actions/plot/scatterplotWithTwoHists.py

        doc="Selector used to determine high SN Objects", default=SnSelector(threshold=2700)
    )
-    lowSNSelector = ConfigurableActionField[VectorAction](
+    lowSNSelector = ConfigurableActionField[SnSelector](


same, but I am happy to have a discussion on it.

natelust · 2022-07-29T21:21:18Z

python/lsst/analysis/tools/actions/plot/scatterplotWithTwoHists.py

@@ -125,25 +82,35 @@ def getOutputSchema(self) -> KeyedDataSchema:

    def __call__(self, data: KeyedData, **kwargs) -> KeyedData:
        results = {}
-        highMaskKey = f'{self.identity or ""}HighSNMask'
+        highMaskKey = f'{self.identity.lower() or ""}HighSNMask'


why did you add this here?

I actually added it in DM-35818 (the ticket I split off with various fixes, mainly for scatter plot). Without it, my tasks fail with ValueError: Task needs keys {'galaxiesHighSNMask', 'galaxiesLowSNMask', 'starsHighSNMask', 'starsLowSNMask'} but they were not found in input. So presumably self.identity is capitalized but the keys are not.

natelust · 2022-08-01T19:13:42Z

python/lsst/analysis/tools/actions/vector/calcBinnedStats.py

+            ("range_minimum", Scalar),
+        )
+
+    @cached_property


I don't think you want a cached_property as these fields are configurable

Hmm. It works as is, because these are meant to be frozen configs, no? Although I suppose that's not enforced as it is elsewhere with Tasks, etc.

natelust · 2022-08-01T20:09:27Z

python/lsst/analysis/tools/actions/vector/calcBinnedStats.py

+class CalcBinnedStatsAction(KeyedDataAction):
+    name_prefix = Field[str](doc="Field name to append stat names to", default="")
+    name_suffix = Field[str](doc="Field name to append to stat names", default="")
+    rangeSelector = ConfigurableActionField[RangeSelector](doc="Range selector")


Just to highlight, this will restrict this field to only work with RangeSelector or a subclass there of

natelust · 2022-10-05T14:05:32Z

python/lsst/analysis/tools/actions/scalar/scalarActions.py

@@ -61,6 +61,16 @@ def __call__(self, data: KeyedData, **kwargs) -> Scalar:
        )


+class ConstantAction(ScalarAction):


Is this just for tests?

Adding more doc string would be useful

I added it just so there was a scalar equivalent to the vector one, but I don't actually use it, no. I can just remove it.

natelust · 2022-10-05T14:20:51Z

python/lsst/analysis/tools/actions/vector/vectorActions.py

@@ -145,6 +144,18 @@ def __call__(self, data: KeyedData, **kwargs) -> Vector:
        return np.array(cast(Vector, result))


+class ConstantValue(VectorAction):


This is easy to get confused with the Scalar one. Additionally, do we need it? Or would it be better to defer to a Vector action that can run generic Scalar ones?

I do need it, yes, but if you think it can be replaced with a generic scalar-to-vector action, that's fine by me.

In KeyedDataActions we have KeyedScalars that does something similar. I think in the spirit of analysis tools where composition is prime focus, maybe a generic VectorAction that turns a ScalarAction into a vector would be best.

Ok, well I'm open to adding that if there's at least one more use case, but since I didn't use the ConstantAction and decided to get rid of it entirely, I'd rather wait until there's a more pressing need to introduce that kind of generic scalar-to-vector action.

natelust · 2022-10-05T14:59:47Z

python/lsst/analysis/tools/analysisMetrics/diffMatchedMetrics.py

+
+    names = ("stars", "galaxies", "all")
+    types = ("unresolved", "resolved", "all")
+


I think this things a getInputSchema

Sort of? It also returns values that are units, not just the keys, and takes mandatory args.

👍 . That said, even though it will not break now, it is supposed to have a getInputSchema method, perhaps you can make one deffer to the other in some way?

As you suggested offline, there is a default getInputSchema and it seems to work fine in the new unit tests, so there's no need to override it (yet).

natelust · 2022-10-05T15:10:58Z

python/lsst/analysis/tools/analysisPlots/diffMatchedPlots.py

+
+class MatchedRefCoaddPositionPlot(MatchedRefCoaddCModelPlot, MatchedRefCoaddDiffPositionTool):
+    def matchedRefDiffContext(self):
+        super(MatchedRefCoaddPositionPlot, self).matchedRefDiffContext()


Why are you using parameterized version of super?

natelust · 2022-10-05T15:13:54Z

python/lsst/analysis/tools/tasks/diffMatchedAnalysis.py

+class DiffMatchedAnalysisConnections(
+    AnalysisBaseConnections,
+    dimensions=("skymap", "tract"),
+    defaultTemplates={"inputName": "diff_matched_truth_summary_objectTable_tract"},


did InputName get changed to something else while I have been sitting on this ticket review?

... I don't think so? I'm not sure what you mean.

taranu force-pushed the tickets/DM-35621 branch 6 times, most recently from 0778ec5 to 5379f8d Compare July 28, 2022 16:32

taranu force-pushed the tickets/DM-35621 branch from 5379f8d to 23be579 Compare August 3, 2022 18:23

taranu force-pushed the tickets/DM-35621 branch 3 times, most recently from 02fceff to 8fdd630 Compare August 17, 2022 17:55

taranu force-pushed the tickets/DM-35621 branch 7 times, most recently from d46b7d5 to 4624524 Compare September 26, 2022 17:23

natelust requested changes Oct 5, 2022

View reviewed changes

taranu force-pushed the tickets/DM-35621 branch 2 times, most recently from 7dd41ae to 6e8c091 Compare October 6, 2022 18:14

taranu force-pushed the tickets/DM-35621 branch 3 times, most recently from 58dad02 to 3b35222 Compare October 17, 2022 15:11

taranu force-pushed the tickets/DM-35621 branch 3 times, most recently from 59ada8b to 7aec3bf Compare October 27, 2022 03:04

natelust approved these changes Oct 28, 2022

View reviewed changes

taranu added 3 commits October 27, 2022 18:26

Add ConstantValue

5e70384

Show S/N: N/A for plotInfo SNText if get fails

90cb668

Add DiffMatchedAnalysisTask, pipeline and dependencies

9a5a8fb

taranu added 2 commits October 27, 2022 18:26

Add test_metrics with TestDiffMatched

f7142de

Fix copyright header

81277b6

taranu force-pushed the tickets/DM-35621 branch from 7aec3bf to 81277b6 Compare October 28, 2022 01:26

taranu merged commit 0fae88a into main Oct 28, 2022

taranu deleted the tickets/DM-35621 branch October 28, 2022 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-35621: Add matched-to-reference table plots and metrics #17

DM-35621: Add matched-to-reference table plots and metrics #17

taranu commented Jul 22, 2022

natelust left a comment

natelust Jul 29, 2022

taranu Oct 5, 2022

natelust Jul 29, 2022

natelust Jul 29, 2022

taranu Oct 5, 2022 •

edited

natelust Aug 1, 2022

taranu Oct 5, 2022

natelust Aug 1, 2022

natelust Oct 5, 2022

natelust Oct 5, 2022

taranu Oct 6, 2022

natelust Oct 5, 2022

taranu Oct 6, 2022

natelust Oct 12, 2022

taranu Oct 12, 2022

natelust Oct 5, 2022

taranu Oct 6, 2022

natelust Oct 12, 2022

taranu Oct 26, 2022

natelust Oct 5, 2022

natelust Oct 5, 2022

taranu Oct 6, 2022

natelust Oct 12, 2022

		@@ -61,6 +61,16 @@ def __call__(self, data: KeyedData, **kwargs) -> Scalar:
		)


		class ConstantAction(ScalarAction):

		@@ -145,6 +144,18 @@ def __call__(self, data: KeyedData, **kwargs) -> Vector:
		return np.array(cast(Vector, result))


		class ConstantValue(VectorAction):


		names = ("stars", "galaxies", "all")
		types = ("unresolved", "resolved", "all")

DM-35621: Add matched-to-reference table plots and metrics #17

DM-35621: Add matched-to-reference table plots and metrics #17

Conversation

taranu commented Jul 22, 2022

natelust 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

taranu Oct 5, 2022 • 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

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

taranu Oct 5, 2022 •

edited