DM-41490: Recover WCS for input with astrometry failures #40

cmsaunders · 2023-12-15T19:30:14Z

No description provided.

parejkoj

There's no test of this functionality: you should be able to make a trivial visitSummary that has a None (or missing) WCS for one detector and test with that.

parejkoj · 2023-12-22T20:19:16Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+                    # This extension does not have information necessary for
+                    # fit. Skip these detections.


I'm not following this comment (I don't know what an extension is in this context). By "these detections", do you mean "this detector/visit"?

"extension" is the gbdes term for one detector from one visit, and it's used throughout the gbdes task. I can change this line to "Skip the detections from this detector for this visit" to make it more clear.

parejkoj · 2023-12-22T20:19:55Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+                    # This extension does not have information necessary for
+                    # fit. Skip these detections.


Same comment as above, with the aded question of why this is duplicated here?

This is duplicated because the sources get loaded two times--once when the source association is done, and again when they are added to the model fitting object.

parejkoj

Sorry, I clicked the button too soon earlier.

parejkoj · 2023-12-22T20:21:52Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+                    # This extension was not fit, but try to recover WCS
+                    genericElements = mapTemplate["EXPOSURE/DEVICE/base"]["Elements"]
+                    mapElements = []
+                    instrument = visitSummary[0].getVisitInfo().getInstrumentLabel()


You should be able to do summary.visitInfo.instrumentLabel, instead of using getters.

summary.visitInfo doesn't work, but I can do visitSummary[0].getVisitInfo().instrumentLabel.

python/lsst/drp/tasks/gbdesAstrometricFit.py

parejkoj · 2023-12-22T20:24:46Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+                        elements = mapTemplate[component]["Elements"]
+                        for element in elements:
+                            for generic, specific in {
+                                "BAND": instrument,


Is instrument really a thing to substitute for band?

It probably shouldn't be, but these lines are trying to replicate how gbdes names mappings, and the instrument name is what ends up here (probably due to this line: https://github.com/lsst/gbdes/blob/4efbc7097dc222cb35d22f8aed79572413aa1a97/include/Instrument.h#L21). A near-term item on my to-do list is to switch to fitting multiple bands at once, so this will probably be addressed there.

parejkoj · 2023-12-22T20:25:30Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+                    genericElements = mapTemplate["EXPOSURE/DEVICE/base"]["Elements"]
+                    mapElements = []
+                    instrument = visitSummary[0].getVisitInfo().getInstrumentLabel()
+                    for component in genericElements:


I really don't understand what this nested set of loops is supposed to do. I assume it's a gbdes internals thing?

Sort of...this loop goes through the generic definitions for the mappings that take you from pixels to sky and uses those to get the names for the maps for a specific extension (i.e. visit/detector). I spent a lot of time trying to get gbdes to do this itself, but couldn't manage it, so this is my replication of how gbdes makes the names.

parejkoj · 2023-12-22T20:26:05Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+                if mapName in wcsf.mapCollection.allMapNames():
+                    mapElements = wcsf.mapCollection.orderAtoms(f"{mapName}/base")
+                else:
+                    # This extension was not fit, but try to recover WCS


Assuming "extension" means "detector/visit": if a detector/visit wasn't fit, how could there be a WCS?

Yes, "extension" means "detector/visit". The detector part of the model is fixed by the sources from this detector on other visits, and the visit part of the model is fixed using the other detectors for the visit. Actually, this makes me realize I need to add a check so that this doesn't fail when there is only one detector.

Scratch the part about what happens when there is only one detector. That will be caught at line 1354.

parejkoj · 2023-12-22T20:27:02Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+                    self.log.warning(
+                        "WCS is None for visit %d, detector %d: this extension will be dropped.",
+                        visit,
+                        detector,
+                    )
+                    continue


Should this be a warning, or an info-level message? Also, saying "extension" in the log is unhelpful for anyone who doesn't know the gbdes internals.

I think a warning is right (from the dev-guide: "WARNING: for conditions that may indicate a problem but that allow continued execution"). I think there should be a note of caution for the resulting WCS.
I changed the message to say "...this extension (visit/detector) will be dropped."

Do you have a way to flag the WCSs that are "reconstructed" in this case in the output, to warn users? A warning message may be appropriate here, but it's not sufficient to tell people that the output WCS is potentially unreliable.

No, neither the WCSs nor the visitSummary tables have a flag for WCS quality. The WCS fit will have higher uncertainty, as it would if there were very few stars to fit, and Eli and I have talked a bit about how to propagate this information.

cmsaunders · 2024-01-08T23:03:36Z

Thanks for the review @parejkoj. I'll add a unit test and update the PR.

parejkoj

You really should pull the contents of some of these for-loops into separate functions. That will make it easier to parse, and at least in one case here, you won't have duplicated code.

parejkoj · 2024-01-11T23:53:29Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

-                )[0]
+                )
+                if len(findExtension) == 0:
+                    # This extension does not have information necessary for
+                    # fit. Skip the detections from this detector for this
+                    # visit.
+                    continue
+                extensionIndex = findExtension[0]
+


Most of the contents of this for-loop are the same as the loop at line 1000: looks like you should have a function for the internals here that gets called in both places.

I looked at this for a while and decided that the two loops are different enough that a function is not going to improve clarity much. I did implement a _find_extension_index() method for a part of the loop that is repeated.

parejkoj · 2024-01-12T00:00:17Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+                    for component in genericElements:
+                        elements = mapTemplate[component]["Elements"]
+                        for element in elements:
+                            element = element.replace("BAND", str(instrument))


I think you need a comment here as to why band and instrument are being mixed.

parejkoj · 2024-01-12T00:01:21Z

tests/test_gbdesAstrometricFit.py

+        )
+
+        # Check that the fit WCS for the extension with input WCS=None returns
+        # finite sky values.


Should it return finite sky values, or correct sky values? Is the test dataset not large enough to get a reasonable fit in this case?

The test data is simulated without the missing WCS, so we don't expect to be able to recover the same input in this test. It's possible to do a whole new simulation, but I don't think it's worthwhile in this case.

parejkoj

Thanks for the cleanups.

cmsaunders force-pushed the tickets/DM-41490 branch from cf3dd58 to f88401a Compare December 15, 2023 19:32

parejkoj reviewed Dec 22, 2023

View reviewed changes

parejkoj requested changes Dec 22, 2023

View reviewed changes

cmsaunders force-pushed the tickets/DM-41490 branch 2 times, most recently from 6941558 to 0509645 Compare January 9, 2024 19:23

parejkoj reviewed Jan 12, 2024

View reviewed changes

Recover WCS for input with astrometry failures

ac40fb2

cmsaunders force-pushed the tickets/DM-41490 branch from c345a11 to ac40fb2 Compare January 17, 2024 15:24

parejkoj approved these changes Jan 17, 2024

View reviewed changes

cmsaunders merged commit 21efbaf into main Jan 19, 2024
3 checks passed

cmsaunders deleted the tickets/DM-41490 branch January 19, 2024 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-41490: Recover WCS for input with astrometry failures #40

DM-41490: Recover WCS for input with astrometry failures #40

cmsaunders commented Dec 15, 2023

parejkoj left a comment

parejkoj Dec 22, 2023

cmsaunders Jan 8, 2024

parejkoj Dec 22, 2023

cmsaunders Jan 8, 2024

parejkoj left a comment

parejkoj Dec 22, 2023

cmsaunders Jan 8, 2024

parejkoj Dec 22, 2023

cmsaunders Jan 8, 2024

parejkoj Dec 22, 2023

cmsaunders Jan 8, 2024

parejkoj Dec 22, 2023

cmsaunders Jan 8, 2024

cmsaunders Jan 8, 2024

parejkoj Dec 22, 2023

cmsaunders Jan 8, 2024

parejkoj Jan 12, 2024

cmsaunders Jan 16, 2024

cmsaunders commented Jan 8, 2024

parejkoj left a comment

parejkoj Jan 11, 2024

cmsaunders Jan 16, 2024

parejkoj Jan 12, 2024

parejkoj Jan 12, 2024

cmsaunders Jan 16, 2024

parejkoj left a comment

		# This extension does not have information necessary for
		# fit. Skip these detections.

DM-41490: Recover WCS for input with astrometry failures #40

DM-41490: Recover WCS for input with astrometry failures #40

Conversation

cmsaunders commented Dec 15, 2023

parejkoj 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

parejkoj 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

cmsaunders commented Jan 8, 2024

parejkoj 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

parejkoj left a comment

Choose a reason for hiding this comment