DM-50376: Add detailed plots by Alex-Broughton · Pull Request #75 · lsst/cp_verify

Alex-Broughton · 2025-05-22T17:39:53Z

No description provided.

aferte

Great work! I have a few comments in the review.
Do you have examples I could look at? One for LSSTCam and one for ComCam or LATISS if possible?
My main worry otherwise is that the general approach of cp verify is to use unified stats and plotting capabilities from analysis_tools but this is implementing its own plots and stats. If @czwa is fine with it, that's fine by me.

aferte · 2025-06-20T18:47:16Z

 import sys

-from lsst.cp.verify.report import main
+from lsst.cp.verify.cpvReport import main


What is the motivation for renaming report? It seems redundant to me, but I also don't mind much - just wondering.

I renamed it so that there is less confusion between the two different "reports"

aferte · 2025-06-20T19:12:33Z

+                    yield from _nested(v)
+                else:
+                    yield v
+        return np.asarray(list(_nested(d)))


Why can't we just use np.array(list(d.values())) instead of this method?

This recurses into d = {'my_favorite_stat' = 1.2345, 'all_stats': {'stat0': 0.0, 'stat1': 1.0, 'stat2': 2.0}} to return [1.2345, 0.0, 1.0, 2.0], although I suspect it's there to handle d = {'detectorNameA': {'ampName0': 0.0, 'ampName1': 1.0}, 'detectorNameB': {'ampName0': 0.0, 'ampName1': -1.0}}.

aferte · 2025-06-20T19:36:22Z

+                    f"Linearity Turnoff:\n{round(linTurnoff, 1)} adu",
+                )
+
+                ax.set_xlim(-.5e-7, 6.5e-7)


A general comment for the rest of the code: is it ok to give specific values for the limits instead of limits dependent on the data? E.g. would these be good limits for LATISS?

For the detailed plots, we want to make sure everything has the same limits, so that when viewing everything together, one can easily find outliers.

aferte · 2025-06-20T19:41:18Z

+    <style>
+        .grid-container {
+            display: grid;
+            grid-template-columns: repeat(15, 1fr);


This (and several other settings below) seems specific to LSSTCam. Is it possible to make it so we can use it for other cameras? I am thinking we will want to use this for LATISS as well in the future. If you prefer not to worry about other cameras, then I am fine but we should clarify in the code name and documentation that this is LSSTCam specific.

The detailed viewer is meant for finding outliers in statistics for large numbers of sensors, like in LSSTCam. I don't think we will need the detailed web pages for LATISS. That is why I separated out the plot generation from the web reports. We can make all plots for every instrument, but we only need to make the web reports for LSSTCam. This would work for AuxTel and ComCam though.

Ok, I updated it so that the web reports are only for LSSTCam. ComCam's done and we won't need the web reports for AuxTel since it is just one sensor.

czwa

Documentation requests, some clarifications where I got confused, and at least one likely typo (the C01/C10 covariance terms).

The concerns mentioned in detailedReport.py:

How long does this take to run to generate plots? The previous version of the cpv_report also attempted to generate all of the plots at once, and it took an infeasible amount of time to run. This led to the analysis_tools-ification, as then we could have tasks to generate all the plots that run as part of the pipeline.
This also solves the issue with getDetailedImages. If the plots are generated as part of the pipeline, then they have butler dataIds themselves, and so each plot type (corresponding to a dataset_type in the butler) can be directly requested from the refs, and used to build the report.
I'm generally worried about diverging cpv reports. I'm happy to switch the existing plots into this report format, as the existing report is very ugly and difficult to navigate. However, I think the base idea of "generate in task, collate by script" is more robust because it keeps most of the work in something that's more parallelizable.

czwa · 2025-06-25T22:45:20Z

+        The location the report will be written to.
+    collections : `list` [`str`]
+        A list of collections to search.
+    **kwargs :


I don't think this is true. doOverwrite will not be set if kwargs['do_overwrite'] is found, and do_copy gets lost entirely.
detectorIds and plotSummaryStats are also not included here.

czwa · 2025-06-25T22:55:38Z

+                    yield from _nested(v)
+                else:
+                    yield v
+        return np.asarray(list(_nested(d)))


This recurses into d = {'my_favorite_stat' = 1.2345, 'all_stats': {'stat0': 0.0, 'stat1': 1.0, 'stat2': 2.0}} to return [1.2345, 0.0, 1.0, 2.0], although I suspect it's there to handle d = {'detectorNameA': {'ampName0': 0.0, 'ampName1': 1.0}, 'detectorNameB': {'ampName0': 0.0, 'ampName1': -1.0}}.

czwa · 2025-06-26T19:13:33Z

+            os.makedirs(f"{self.output}/{raft}/{bay}", exist_ok=True)
+
+    def plotLinearizerStatistics(self):
+        self.plotIdx += 1


This leaves plots indexed starting at 1, not zero, if I'm thinking through the logic correctly, right?

That's right. I wanted to leave it indexed at zero as it is more human-readable

czwa · 2025-06-26T19:32:00Z

+
+        self.refs = refs
+
+        for ref in tqdm(self.refs, total=len(self.refs), desc="LINEARIZER"):


This needs a comment explaining what's happening. It's reasonably simple to follow once I know what tqdm does, but I had to google it (although it is in the default stack install, so maybe it's just me).

czwa · 2025-06-26T21:36:56Z

+    -------
+    list of matplotlib.patches.Rectangle objects
+    """
+    transform = det.getTransform(cameraGeom.PIXELS, cameraGeom.FOCAL_PLANE)


This doesn't correctly handle rotations. See: https://github.com/lsst/analysis_tools/blob/main/python/lsst/analysis/tools/actions/plot/focalPlanePlot.py#L465

In fact, I think I'm going to suggest that if these functions are essential, they should be factored into methods on FocalPlaneGeometryPlot, so that there's only one code path doing this.

czwa · 2025-06-26T21:49:02Z

+        The location the report will be written to.
+    collections : `list` [`str`]
+        A list of collections to search.
+    **kwargs :


Same comment as above on this docstring not matching the code.

czwa · 2025-06-26T21:54:29Z

+            raise RuntimeError(f"The shared directory {self.sharedPath} does not exist.")
+        if not os.path.isdir(self.sharedPath + "/images"):
+            raise RuntimeError(f"The shared directory {self.sharedPath} does not follow the expected "
+                               "organizational structure, please refer to DMTN-222 for more information.")


Is there DMTN-222 ticket branch with this info?

czwa · 2025-06-26T22:02:39Z

+            for file in files:
+                if file.endswith('.png') and not file.endswith('_fp.png'):
+                    try:
+                        pattern = re.compile(r"(step_\d{2}[a-zA-Z]?)-([a-zA-Z0-9_-]+)_(\d{2,3})\.png")


I have concerns.

czwa · 2025-06-26T22:51:15Z

    Parameters
    ----------
-    statDictA, statDictB : `dict` [`str`, `dict`]
+    statDictA, statDictB: `dict` [`str`, `dict`]


The dev guide claims these are important spaces: https://developer.lsst.io/python/numpydoc.html#parameters

czwa · 2025-06-26T22:51:54Z

    Raises
    ------
-    RuntimeError :
+    RuntimeError:


But the dev guide also suggests this shouldn't have a : at all, so I'm learning new things too: https://developer.lsst.io/python/numpydoc.html#parameters

Alex-Broughton force-pushed the tickets/DM-50376 branch from 5586c3d to fb684fd Compare May 22, 2025 21:03

Alex-Broughton requested review from aferte and czwa June 19, 2025 20:25

aferte reviewed Jun 20, 2025

View reviewed changes

czwa reviewed Jun 26, 2025

View reviewed changes

Alex-Broughton added 4 commits December 1, 2025 14:45

Add detailed plotter

8b8d065

Add commandline configs

7d89bea

Add detailed reporter

f2e8e92

rebase

ddaedc8

Alex-Broughton force-pushed the tickets/DM-50376 branch from 04d8d39 to ddaedc8 Compare April 3, 2026 22:12


		self.refs = refs

		for ref in tqdm(self.refs, total=len(self.refs), desc="LINEARIZER"):

Conversation

Alex-Broughton commented May 22, 2025

Uh oh!

aferte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

czwa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aferte left a comment •

edited

Loading