DM-40763: Modify region definition config in extended_psf #837

bazkiaei · 2023-09-28T22:14:33Z

No description provided.

arunkannawadi · 2023-10-19T22:09:26Z

python/lsst/pipe/tasks/extended_psf.py

+    """
+    detectors = ListField[int](
+        doc="A list containing the detectors IDs.",
+        default=None,


If the default value is None, then you should also set optional=True so that the default value is a valid one.

Is there a reason for this to be an instance of a Config?

Ah, is that because you want to specify this as the itemtype in the ConfigDictField?

Added optional=True as suggested. You're correct on your second point here.

Wait, in find_region_for_detector, you check for membership without any checks (I can't tell yet if it has been checked previously before it gets there). May be this should be an empty list by default instead of None (in which case, you can remove the optional=True.

That's a nice idea. Modified as you suggest.

arunkannawadi · 2023-10-19T22:49:11Z

python/lsst/pipe/tasks/extended_psf.py

@@ -280,7 +325,7 @@ class StackBrightStarsConfig(Config):
    )
    stacking_statistic = ChoiceField[str](
        doc="Type of statistic to use for stacking.",
-        default="MEANCLIP",
+        default="MEDIAN",


This change seems out of scope for this ticket. It should remain the same in this ticket.

It was during testing of this ticket that we discovered that MEDIAN produces superior results. As it's only a 1-line change, I thought it best to put it here. An alternative is to move it to a separate commit. We can also remove it to put this 1-liner on another ticket if you think it optimal.

Definitely a separate commit please. If you could put on Jira what you mean by superior results (maybe you already have), that would be good.

Definitely more details, please! Stacking can be subtle, and medians are deceptively tempting

My preference for a different ticket also comes when we look at the CHANGELOG between versions, it wouldn't be obvious at all that a (perhaps significant) algorithmic change was made here looking at the title of the ticket.

I'll go with the median opinion on this thread and relegate this change to a subsequent ticket.

arunkannawadi · 2023-10-20T17:00:22Z

python/lsst/pipe/tasks/extended_psf.py

 from lsst.pipe.base import PipelineTaskConfig, PipelineTaskConnections, Struct, Task
 from lsst.pipe.tasks.coaddBase import subBBoxIter
 from lsst.pipe.base.connectionTypes import Input, Output


+def find_region_for_detector(det_id, detectors_focal_plane_regions):


This could become a static method of ExtendedPsf unless you plan on using it more generally as a utility function (which I don't think is the case)

It's also called inside MeasureExtendedPsfTask. I think we'd imagined this may be a somewhat convenient utility script allowing for a trivial lookup between detector ID and region ID. Perhaps that's not required?

Oh, I see it now. I guess it's fine as it is.

arunkannawadi · 2023-10-20T17:01:27Z

python/lsst/pipe/tasks/extended_psf.py

+
+    Parameters
+    ----------
+    det_id : int


Suggested change

det_id : int

det_id : `int`

arunkannawadi · 2023-10-20T17:02:20Z

python/lsst/pipe/tasks/extended_psf.py

+    det_id : int
+        The detector ID.
+
+    detectors_focal_plane_regions : dict


Suggested change

detectors_focal_plane_regions : dict

detectors_focal_plane_regions : `dict` [`str`, `int`]

arunkannawadi · 2023-10-20T17:17:44Z

python/lsst/pipe/tasks/extended_psf.py

        List of detector IDs that define the focal plane region over which this
        extended PSF model has been built (and can be used).
    """

    extended_psf_image: MaskedImageF
-    detector_list: List[int]
+    region_detectors: DetectorsInRegion


Doesn't affect the code in runtime, but shouldn't this be ListField as well, based on the above. It's inconsistent with the docstring above.

Actually, I think the docstring is incorrect here. Modifying here (and elsewhere) accordingly to lsst.pipe.tasks.extended_psf.DetectorsInRegion.

arunkannawadi · 2023-10-20T18:22:32Z

python/lsst/pipe/tasks/extended_psf.py

        doc=(
-            "Mapping from detector IDs to focal plane region names. If empty, a constant extended PSF model "
+            "Mapping from focal plane region names to detector IDs. If empty, a constant extended PSF model "


There should be a check in a validate method so that the values of the dictionaries don't contain a shared entity. This is mainly to prevent a user error when dealing with 100s of detectors that a detector is initially marked as say "W" and then marked as "SW" without removing it from "W". Or could a single detector be in two regions? With the previous version, such a thing would have not been possible.

Also, I see that the region_name gets homogenized here. So yet another job for the validate method could be to ensure there are no keys like "NW" and "nw". That's a less likely user error compared to what I mentioned in the previous comment.

There's actually nothing preventing a detector being used in multiple regions, and this is an interesting point to raise. I actually don't know if allowing or preventing such an action is useful scientifically.

I can imagine special cases, for example, a camera with a central detector and the user wishes to use the central detector in all sectors of an equally segmented focal plane. Is this optimal? I'm not sure - but I can imagine some may wish to experiment with it?

I see the advantage, but I can't be sure if that's the right thing to do. It should be documented that a detector could be in more than one region and we just have to assume that that's what the user intended.

Added a note on this into the associated docstring.

arunkannawadi · 2023-10-20T18:30:08Z

python/lsst/pipe/tasks/extended_psf.py

-        # corresponding focal plane region, make sure we keep track of
-        # it (eg to raise a warning only once)
+        if self.config.detectors_focal_plane_regions:
+            self.detectors_focal_plane_regions = self.config.detectors_focal_plane_regions


I don't think an if statement is required. It's better to have an empty dict (the default value) than have an AttributeError later.

Agreed - removed the if and unindented the config setting.

arunkannawadi · 2023-10-20T18:33:04Z

python/lsst/pipe/tasks/extended_psf.py

@@ -482,10 +520,24 @@ def select_detector_refs(self, ref_list):
            region_ref_list[region_name].append(dataset_handle)
        return region_ref_list

+    def _find_detectors(self, bss_ref_list):


Looks like this should just be a staticmethod.

Added the @staticmethod decorator and removed self parameter. However, I'm not sure we need this method any longer, and it may be a vestigial artefact from our development phase on this ticket. Keen to hear from @bazkiaei on this.

Yes, you are right @leeskelvin . This method was created to generate a region including all existing detectors as the default region when the user do not provide regions, but since we changed our approach for that we do not need this anymore and we can remove this.

arunkannawadi · 2023-10-20T19:03:00Z

python/lsst/pipe/tasks/extended_psf.py

+    raise KeyError(
+        "Detector %d is not included in any focal plane region.",
+        det_id,
+    )


I don't know how much it matters in practice, but it might be better invert the mapping once upfront (detector ID as key and region as value, as it was before) insead of looping over a dict and then through a list for every query.

In practice it seemingly adds very little extra runtime. But yes, I agree a cleaner way would be to construct an inverted lookup internal to these classes at runtime. We have a future refactor ticket which would be a good place to defer this effort to, so I'll make a note of that there.

arunkannawadi · 2023-10-24T14:46:56Z

python/lsst/pipe/tasks/extended_psf.py

 from lsst.pipe.base.connectionTypes import Input, Output
+from lsst.pipe.tasks.coaddBase import subBBoxIter


The rest of the imports appear to be sorted as isort would sort them, so pushing this down by one line seems unnecessary.

This change was made by isort, so I suspect this is the correct location for this import in isort-land.

Oh, my bad. I misread it initially.

arunkannawadi · 2023-10-24T14:54:20Z

python/lsst/pipe/tasks/extended_psf.py

                    "'detectors_focal_plane_regions'",
                )
-                self.regionless_dets.append(det_id)
+                self.regionless_dets.append(detector_id)


Are we expecting the code to execute this except block frequently, if only a few detectors are assigned to regions? Is the typical use case expected to have all (at least most) detectors assigned to regions, or only for a handful number of detectors?

This except will only trigger if a given detector was not specified by the user in any of the specified regions. This should hopefully be a rare case - i.e., if I'm splitting a visit into regions, I should hope to specify all detectors. The obvious warning should also be useful in this case, in case this is an oversight by the end-user.

arunkannawadi · 2023-10-24T14:54:52Z

python/lsst/pipe/tasks/extended_psf.py

+            if detector_id not in detectors:
+                detectors.append(detector_id)
+        return detectors
+


This method is going away, right?

bazkiaei force-pushed the tickets/DM-40763 branch 2 times, most recently from 29cedfe to b49c4eb Compare October 8, 2023 06:24

bazkiaei marked this pull request as ready for review October 8, 2023 06:30

bazkiaei force-pushed the tickets/DM-40763 branch from b49c4eb to 6e44ebf Compare October 19, 2023 13:45

arunkannawadi approved these changes Oct 20, 2023

View reviewed changes

leeskelvin force-pushed the tickets/DM-40763 branch from 6e44ebf to 281496d Compare October 20, 2023 21:27

arunkannawadi approved these changes Oct 24, 2023

View reviewed changes

leeskelvin force-pushed the tickets/DM-40763 branch from 281496d to 8616186 Compare October 24, 2023 17:44

Modify region definition config in extended_psf

fb1e298

leeskelvin force-pushed the tickets/DM-40763 branch from 8616186 to fb1e298 Compare October 24, 2023 22:15

leeskelvin merged commit 3b70695 into main Oct 24, 2023
2 checks passed

leeskelvin deleted the tickets/DM-40763 branch October 24, 2023 22:16

	detectors_focal_plane_regions : dict
	detectors_focal_plane_regions : `dict` [`str`, `int`]

		from lsst.pipe.base.connectionTypes import Input, Output
		from lsst.pipe.tasks.coaddBase import subBBoxIter

DM-40763: Modify region definition config in extended_psf #837

DM-40763: Modify region definition config in extended_psf #837

Conversation

bazkiaei commented Sep 28, 2023

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

leeskelvin Oct 20, 2023 • 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

leeskelvin Oct 20, 2023 • 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

arunkannawadi Oct 20, 2023 • 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

leeskelvin Oct 20, 2023 •

edited

leeskelvin Oct 20, 2023 •

edited

arunkannawadi Oct 20, 2023 •

edited