DM-34625: Add sky coverage plots of visit summary quantities to analysis_tools #51

laurenam · 2022-12-15T03:03:07Z

No description provided.

cmsaunders

A few overall comments:

Can you add the units for the colorbars?
From the naming, i.e. VisitCoveragePlot, it isn't really clear that the plots are for multiple visits, and I think users might get mixed up with the visit-level plots. Maybe something like MultiVisitCoveragePlot?
Can you explain the "focalPlane" vs "raDec" plots options somewhere? Without looking at the examples, it isn't obvious that the focalPlane plot will give one number per detector, while the raDec is binned in ra/Dec.

I didn't go through the big makePlot function as closely as I would have liked to, but I wanted to get this back to you before I go on break. I would be happy to look at it again, but if you want to get this merged, I will let you make the call on that.

cmsaunders · 2022-12-19T20:27:17Z

ups/analysis_tools.table

 setupRequired(daf_butler)
+setupRequired(geom)


Did you end up using this or meas_algorithms, or are these existing dependencies that should have already been included? I didn't find them anywhere in this PR.

Oh, yeah. When I realized I had to update this table, I figured I might as well check if other dependencies had been omitted. I found these (both occur in catalogMatch.py).

cmsaunders · 2022-12-19T20:42:38Z

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

+    xNumBins : `int`, optional
+        The number of bins in along the x-axis to use in the binning.
+    yNumBins : `int`, optional
+        The number of bins in along the y-axis to use in the binning.


bins in along -> bins along, and to use in the binning is redundant. Same thing at ln 682.

cmsaunders · 2022-12-19T22:09:47Z

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

+            dataId = DataCoordinate.standardize(dataId, universe=butlerQC.registry.dimensions)
+        plotInfo = self.parsePlotInfo(inputs, dataId)
+        data = self.loadData(inputs["data"])
+        if "skymap" in inputs.keys():


Can you make the if statements for setting skymap, camera, and makeWarpConfig all be written the same way? I.e. all one-liners or all multi-line statements.

cmsaunders · 2022-12-21T18:32:06Z

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

+from ..interfaces import AnalysisPlot
+
+
+class BaseVisitCoveragePlot(AnalysisPlot):


Maybe there has been discussion of this elsewhere, but is there a strong reason to have these in their own file, instead of in analysisPlots.py? I also think it would be helpful to have the filename be distinctly different from actions/plot/coveragePlot.py, as navigating analysis_tools is already quite difficult.

I don't know of any official word on this, but I am going on the assumption that making analysisPlots a subdirectory came with the intent of having the freedom/flexibility to separate the variously flavored actions out into separate files (where here the "flavor" is an x,y-plane "coverage"). Pilling more disparate plots classes into the generically named analysisPlots.py is more confusing to me, so I'm going stick with this for now (but will happily accept a refactor if one is more concretely defined and affected!).

Taking on your other naming suggestion, the other file is now called actions/plot/multiVisitCoveragePlot.py, so hopefully that helps at least a bit in distinguishing the two...but I'm totally with you on the difficulties in navigating this repo!

cmsaunders · 2022-12-21T18:40:14Z

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

+    tickFmt : `matplotlib.ticker.FormatStrFormatter`
+       The tick formatter to use with matplotlib functions.
+    """
+    if np.abs(value) >= 10000:


I think you could write a formula for this, maybe something like

x = 4 - np.floor(np.log(np.abs(value))) tickFmt = FormatStrFormatter(f"%.{x}f")

but I didn't check this, and it's up to you.

I see where you're going...but I just can't get it to work. I tried with your incantation, but it required special casing to the point that what I have (despite the mess) is actually more readable (and has been battle tested to work as desired). Given that it's a "_private" function, I'm going to leave it as is (but would be very open to adapting another incantation that is more streamlined, but still works!, if one is suggested).

cmsaunders · 2022-12-21T18:40:34Z

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

+    Parameters
+    ----------
+    value : `float`
+        The the value used to determine the apporpriate formatting.


apporpriate -> appropriate

cmsaunders · 2022-12-21T18:44:22Z

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

+        optional=True,
+    )
+    projection = Field[str](
+        doc="Projection to plot.  Currently only raDec and focalPlane are permitted.",


raDec and focalPlane should be in quotes for clarity.

cmsaunders · 2022-12-21T18:47:30Z

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

+        default="raDec",
+    )
+    nBins = Field[int](
+        doc="Number of bins to use for the effective plot range in the raDec projection.",


A little unclear. Maybe "Number of bins to use in the effective plot range in the spatial direction. Only used in the raDec projection."?

cmsaunders · 2022-12-21T18:55:24Z

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

+        "(regardless if they have any data in them).",
+        default=False,
+    )
+    raDecLimitsDict = DictField[str, float](


Can you just make the default None, and not have doUseRaDecLimits, then only use this if it's not None?

cmsaunders · 2022-12-21T18:59:05Z

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

+            if isScalar and typ != Scalar:
+                raise ValueError(f"Data keyed by {name} has type {colType} but action requires type {typ}.")
+
+        if "medianE" in self.parametersToPlotList:


Do you need more statements like this for all the other parameters to plot? Could you remove vectorsToLoadList and just have a dict somewhere that gives the vectors needed? I'm a little concerned by the interplay between having to set vectorsToLoad corresponding to the parametersToPlot.

Yeah, I'm not thrilled about the interplay either, but can't think of a better way. This is the only metric (at the moment) that is computed based on two entries in the table, so they can't be kept specifically in sync, so I added this check...but I believe it's really the only one required at present. (I did add one other validation on the entries in raDecLimitsDict if that config is overridden.)

This is to enable use of this functionality in other plot actions.

The coverage plots are based on the data in ccdVisitTable and are thus "by collection". Each visit gets one entry per detector. Parameter values can be plotted in either RA/Dec or Focal Plane projections.

laurenam

Can you add the units for the colorbars?

Added (where the parameters are not unitless).

From the naming, i.e. VisitCoveragePlot, it isn't really clear that the plots are for multiple visits, and I think users might get mixed up with the visit-level plots. Maybe something like MultiVisitCoveragePlot?

Suggestion adopted and action/plot filename changed to multiVisitCoveragePlot.py.

Can you explain the "focalPlane" vs "raDec" plots options somewhere? Without looking at the examples, it isn't obvious that the focalPlane plot will give one number per detector, while the raDec is binned in ra/Dec.

They both plot the same data, just in different "projections", which I hoped would be clear with a config name of projection. To (hopefully) help, I added the line:
"In either case, one point is plotted per visit/detector combination."
to the docstring.

laurenam · 2022-12-22T00:03:16Z

ups/analysis_tools.table

 setupRequired(daf_butler)
+setupRequired(geom)


Oh, yeah. When I realized I had to update this table, I figured I might as well check if other dependencies had been omitted. I found these (both occur in catalogMatch.py).

laurenam · 2022-12-27T02:30:14Z

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

+from ..interfaces import AnalysisPlot
+
+
+class BaseVisitCoveragePlot(AnalysisPlot):


I don't know of any official word on this, but I am going on the assumption that making analysisPlots a subdirectory came with the intent of having the freedom/flexibility to separate the variously flavored actions out into separate files (where here the "flavor" is an x,y-plane "coverage"). Pilling more disparate plots classes into the generically named analysisPlots.py is more confusing to me, so I'm going stick with this for now (but will happily accept a refactor if one is more concretely defined and affected!).

Taking on your other naming suggestion, the other file is now called actions/plot/multiVisitCoveragePlot.py, so hopefully that helps at least a bit in distinguishing the two...but I'm totally with you on the difficulties in navigating this repo!

laurenam · 2022-12-27T02:54:34Z

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

+    tickFmt : `matplotlib.ticker.FormatStrFormatter`
+       The tick formatter to use with matplotlib functions.
+    """
+    if np.abs(value) >= 10000:


I see where you're going...but I just can't get it to work. I tried with your incantation, but it required special casing to the point that what I have (despite the mess) is actually more readable (and has been battle tested to work as desired). Given that it's a "_private" function, I'm going to leave it as is (but would be very open to adapting another incantation that is more streamlined, but still works!, if one is suggested).

laurenam · 2022-12-27T21:44:52Z

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

+            if isScalar and typ != Scalar:
+                raise ValueError(f"Data keyed by {name} has type {colType} but action requires type {typ}.")
+
+        if "medianE" in self.parametersToPlotList:


Yeah, I'm not thrilled about the interplay either, but can't think of a better way. This is the only metric (at the moment) that is computed based on two entries in the table, so they can't be kept specifically in sync, so I added this check...but I believe it's really the only one required at present. (I did add one other validation on the entries in raDecLimitsDict if that config is overridden.)

arunkannawadi · 2023-10-13T17:03:46Z

Coming in here super late, but I don't see CcdVisitTableAnalysisTask called anywhere (neither in this PR not in the main branch now). I see that the coveragePlots.yaml that Lauren attached to the Jira ticket uses it, but I don't see it in the pipelines/ folder here. Does that mean we no longer use this, and that it might be bitrotting?

laurenam · 2023-10-13T20:40:47Z

Sigh...indeed, you are correct (and thank you very much for pointing this out!) I definitely still want the ability to make these plots, so I guess I should make a ticket to (fix any bitrot) and add a pipeline to the repo. Do you know how tasks that aren't necessarily run in the "core" pipelines get tested/CI'd?

arunkannawadi · 2023-10-13T20:52:42Z

Oh, I definitely want to reuse this task but found that I had to comment out one line to be able to run it (hence the bitrot comment). I think having a pipeline that could be run would be a good start. If you want it CI'd, you probably need a test with made up data similar to this one: https://github.com/lsst/analysis_tools/blob/main/tests/test_catalogMatch.py

laurenam · 2023-10-13T21:18:11Z

Ah, ok, thanks! How would you like to proceed? Shall I make a ticket as I suggest above, or will your pipeline & bitrot fix(?) be suitable (modulo the ci option...)?

arunkannawadi · 2023-10-13T21:35:06Z

I think you should add the coveragePlots.yaml file to pipelines/ regardless (unless you have strong reasons not to?). That's likely to happen faster than my new ticket (DM-41134) but I am okay with whatever happens first.

laurenam · 2023-10-13T21:38:07Z

Sounds good. I'll try to get to that soon...ish!

laurenam force-pushed the tickets/DM-34625 branch 10 times, most recently from ed41db8 to 45f22ef Compare December 19, 2022 01:26

laurenam requested a review from cmsaunders December 19, 2022 02:12

cmsaunders approved these changes Dec 21, 2022

View reviewed changes

Factor out projection plotting with binning

1cb1d65

This is to enable use of this functionality in other plot actions.

laurenam force-pushed the tickets/DM-34625 branch 2 times, most recently from 859dc3b to 73608d8 Compare December 27, 2022 04:23

Add multi-visit-level coverage plots

5623e10

The coverage plots are based on the data in ccdVisitTable and are thus "by collection". Each visit gets one entry per detector. Parameter values can be plotted in either RA/Dec or Focal Plane projections.

laurenam force-pushed the tickets/DM-34625 branch from 73608d8 to 5623e10 Compare December 27, 2022 21:45

laurenam commented Dec 27, 2022

View reviewed changes

laurenam merged commit 25bc913 into main Dec 27, 2022

laurenam deleted the tickets/DM-34625 branch December 27, 2022 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-34625: Add sky coverage plots of visit summary quantities to analysis_tools #51

DM-34625: Add sky coverage plots of visit summary quantities to analysis_tools #51

laurenam commented Dec 15, 2022

cmsaunders left a comment

cmsaunders Dec 19, 2022

laurenam Dec 22, 2022

cmsaunders Dec 19, 2022

cmsaunders Dec 19, 2022

cmsaunders Dec 21, 2022

laurenam Dec 27, 2022

cmsaunders Dec 21, 2022

laurenam Dec 27, 2022

cmsaunders Dec 21, 2022

cmsaunders Dec 21, 2022

cmsaunders Dec 21, 2022

cmsaunders Dec 21, 2022

cmsaunders Dec 21, 2022

laurenam Dec 27, 2022

laurenam left a comment

laurenam Dec 22, 2022

laurenam Dec 27, 2022

laurenam Dec 27, 2022

laurenam Dec 27, 2022

arunkannawadi commented Oct 13, 2023

laurenam commented Oct 13, 2023

arunkannawadi commented Oct 13, 2023

laurenam commented Oct 13, 2023

arunkannawadi commented Oct 13, 2023

laurenam commented Oct 13, 2023

		from ..interfaces import AnalysisPlot


		class BaseVisitCoveragePlot(AnalysisPlot):

		setupRequired(daf_butler)
		setupRequired(geom)

		setupRequired(daf_butler)
		setupRequired(geom)

DM-34625: Add sky coverage plots of visit summary quantities to analysis_tools #51

DM-34625: Add sky coverage plots of visit summary quantities to analysis_tools #51

Conversation

laurenam commented Dec 15, 2022

cmsaunders 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

laurenam 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

arunkannawadi commented Oct 13, 2023

laurenam commented Oct 13, 2023

arunkannawadi commented Oct 13, 2023

laurenam commented Oct 13, 2023

arunkannawadi commented Oct 13, 2023

laurenam commented Oct 13, 2023