DM-39776: Add global astrometric fit task #30

cmsaunders · 2023-10-17T16:10:52Z

No description provided.

arunkannawadi · 2023-10-20T20:37:30Z

Would you be opposed to blackening the file? I'm thinking of adding black and isort formatting on DM-39243.

cmsaunders · 2023-10-20T22:08:20Z

Hmm, I don't have very strong feelings about using black, but would it mean the code has to be black compliant every time you make a commit? From my experience with analysis_tools, I do find that frustrating during development.

timj · 2023-10-20T22:28:07Z

Once a repo is black-ified it has to stay that way and GitHub actions have to enforce it. This is a per-repo decision that should generally include discussions within the science pipelines team.

arunkannawadi · 2023-10-20T22:31:04Z

I'll ask more broadly on Monday, but this is the only other file that has work in progress and one that I'm not working on.

arunkannawadi · 2023-10-20T22:31:49Z

And no, I don't mean pre-commit. So you can still push code without it being black compliant, but you'll have to make it compliant before merging to main.

laurenam · 2023-12-10T21:04:41Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

@@ -619,6 +625,8 @@ def _get_exposure_info(
            data comes from the same instrument.
        refEpoch : `float`
            Epoch of the reference objects in MJD.
+        fieldRegions : `dict` of `lsst.sphgeom.ConvexPolygon`


`dict` [`KEY?`, `lsst.sphgeom.ConvexPolygon`]

https://developer.lsst.io/python/numpydoc.html#dictionary-types

laurenam · 2023-12-10T21:04:44Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

@@ -619,6 +625,8 @@ def _get_exposure_info(
            data comes from the same instrument.
        refEpoch : `float`
            Epoch of the reference objects in MJD.
+        fieldRegions : `dict` of `lsst.sphgeom.ConvexPolygon`
+            Dictionary of regions encompassing each group of input visits.


What is the key?

laurenam · 2023-12-10T21:11:31Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

@@ -496,9 +500,10 @@ def run(
        )

        # Add the reference catalog to the associator
-        medianEpoch = astropy.time.Time(exposureInfo.medianEpoch, format="decimalyear").mjd
+        medianEpoch = astropy.time.Time(exposureInfo.medianEpoch, format='decimalyear').mjd


laurenam · 2023-12-10T21:15:11Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+              (decErr, "dec", 1),
+              (raPMErr, "pmra", 3),
+              (decPMErr, "pmdec", 4),
+              (parallaxErr, "parallax", 2)


Why remove the trailing comma?

laurenam · 2023-12-10T21:15:50Z

python/lsst/drp/tasks/gbdesAstrometricFit.py


-__all__ = ["GbdesAstrometricFitConnections", "GbdesAstrometricFitConfig", "GbdesAstrometricFitTask"]
+__all__ = ['GbdesAstrometricFitConnections', 'GbdesAstrometricFitConfig', 'GbdesAstrometricFitTask',
+           'GbdesGlobalAstrometricFitTask']


Why " -> '?

laurenam · 2023-12-10T21:16:00Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

@@ -111,11 +115,11 @@ def _make_ref_covariance_matrix(
        for i, pi in enumerate(positionParameters):
            for j, pj in enumerate(positionParameters):
                if i == j:
-                    cov[:, k] = (refCat[f"{pi}Err"] ** 2 * inputUnit**2).to_value(units[j] * units[j])
+                    cov[:, k] = ((refCat[f'{pi}Err'].value)**2 * inputUnit**2).to(units[j] * units[j]).value


Again, why " -> ' (here and below)?

And why does this one need the to_value -> .value change but the two below don't?

laurenam · 2023-12-19T18:47:08Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

@@ -750,11 +751,7 @@ def _get_exposure_info(
            ras.append(raDec.getRa().asRadians())
            decs.append(raDec.getDec().asRadians())
            if fieldRegions:


You check if fieldRegions: here, but if fieldRegions is None: below. Is there a reason for the inconsistency?

No, I just can never remember which one is preferred and random-walk between them.

laurenam · 2023-12-19T18:57:22Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

-        extensionVisits.append(-1)
-        extensionDetectors.append(-1)
-        extensionType.append("REFERENCE")
+        # not used. There needs to be a separate catalog for each field (?)


Is this still a ??

No, removed it.

laurenam · 2023-12-21T01:53:49Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+        exposureInfo = pipeBase.Struct(
+            visits=visits, detectors=detectors, ras=ras, decs=decs, medianEpoch=medianEpoch
+        )
+


This seems to directly duplicate the lines above...

Weird--maybe a rebasing failure?

laurenam · 2023-12-21T01:55:22Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+        center : `lsst.geom.SpherePoint`, optional
+            Center of the circle in which to load reference objects.
+        radius : `lsst.sphgeom._sphgeom.Angle`, optional
+            Radius of the circle in which to load reference objects.


Note that center must also be set and is ignored if region is not None?

laurenam · 2023-12-21T01:56:50Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+        associations : `wcsfit.FoFClass`, optional
+            Object to which to add the catalog of reference objects.
+        center : `lsst.geom.SpherePoint`, optional
+            Center of the circle in which to load reference objects.


Note that radius must also be set? Also that this will be ignored if region is set??

laurenam · 2023-12-26T23:11:02Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+            WCS-fitting object.
+        sourceDict : `dict`
+            Dictionary containing the source centroids for each visit.
+        extensionInfo : `lsst.pipe.base.Struct`


Document the struct parameters: https://developer.lsst.io/python/numpydoc.html#struct-types

laurenam · 2023-12-26T23:11:35Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+        ----------
+        wcsf : `wcsfit.WCSFit`
+            WCS-fitting object.
+        sourceDict : `dict`


dict [?, ?]?

I did

sourceDict : `dict` [`int`, [`int`, [`str`, `list` [`float`]]]]

I'm not sure if getting this in-depth is the best practice, but I think it's accurate.

laurenam · 2023-12-26T23:18:01Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+
+        self.log.info("Fit the WCSs")
+        # Set up a YAML-type string using the config variables and a sample
+        # visit


.

What is this for?

Added "which will be used to tell gbdes what form of model to fit."

laurenam · 2023-12-26T23:18:54Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+        self.log.info("Fit the WCSs")
+        # Set up a YAML-type string using the config variables and a sample
+        # visit
+        inputYAML = self.make_yaml(inputVisitSummaries[0])


laurenam · 2023-12-27T00:05:47Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+    )
+    outputCatalog = pipeBase.connectionTypes.Output(
+        doc=(
+            "Source table with stars used in fit, along with residuals in pixel coordinates and tangent "


"stars" -> "sources"?

laurenam · 2024-01-04T21:39:05Z

tests/test_gbdesAstrometricFit.py

+            record.set(raErrKey, 0.000001)
+            record.set(decErrKey, 0.000001)
+            record.set(pmraErrKey, 1e-10)
+            record.set(pmdecErrKey, 1e-10)


Why are these changing?

I think I was having trouble with the fit being accurate enough. In the mean time, I have improved another aspect for the fitting though, so I should test this again to see if this is still necessary.

Maybe because of other changes since this PR started, this change doesn't seem to be necessary. I have to run jenkins to check macos too though.

laurenam · 2024-01-06T21:16:27Z

tests/test_gbdesAstrometricFit.py

+                starRAs = np.delete(starRAs, neighbors)
+                starDecs = np.delete(starDecs, neighbors)
+                cls.nStars = len(starRAs)
+                starIds = np.arange(cls.nStars)


This block is duplicated above. Worth making it it's own function?

laurenam · 2024-01-06T21:38:10Z

tests/test_gbdesAstrometricFit.py

+
+        for region in self.fieldRegions.values():
+            refCat, refCov = self.task._load_refcat(
+                self.refObjectLoader, self.extensionInfo, region=region, epoch=2015


Isn't epoch usually expressed in MJD (or do you really mean 1864? 😉)

laurenam · 2024-01-12T23:17:18Z

tests/test_gbdesAstrometricFit.py

+        # Set random seed
+        np.random.seed(1234)
+
+        # Fraction of simulated stars in the reference catalog and science


What is the meaning/purpose of "simulated" stars here?

I moved this block later so it should make more sense.

laurenam · 2024-01-12T23:19:34Z

tests/test_gbdesAstrometricFit.py

+
+    @classmethod
+    def _make_isolatedStars(cls, allStarIds, allStarRAs, allStarDecs, trueWCSs, inScienceFraction):
+        # Take a subset of the simulated data


I'm having trouble understanding what is meant by "simulated data" for these tests and how these are used vs. the real data you are also using (so it's hard to get a feel for how much of the task is getting tested). A few words of explanation would go a long way (for me at least!)

I think these were notes to myself that I missed converting into docs. I added full documentation here, plus a short explanation in the class documentation for the parent class:

This class tests `GbdesAstrometricFit` using real `visitSummaryTable`s from HSC RC2 processing, with simulated sources for those visits.

The only real data in the tests is the visitSummaries. The sources and true object positions are all simulated from some "true WCS", which is known to be realistic given the input information in the visitSummaries. The overarching test of the task, which happens in test_run(), checks that, given the inputSummaries and simulated source catalogs, you can recover the "trueWCS" to a certain accuracy.

laurenam · 2024-01-20T20:36:50Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

@@ -721,6 +750,16 @@ def _get_exposure_info(
            raDec = visitInfo.getBoresightRaDec()
            ras.append(raDec.getRa().asRadians())
            decs.append(raDec.getDec().asRadians())
+            if fieldRegions:
+                inField = [r for r, region in fieldRegions.items() if region.contains(raDec.getVector())]


I'm not sure why breaking as soon as len(inField) > 1 doesn't count as the same sanity check, but ok.

laurenam · 2024-01-20T20:41:52Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

@@ -1407,3 +1477,496 @@ def _compute_model_params(self, wcsf):
            modelParams[key] = np.array(value)

        return modelParams
+
+
+class GbdesGlobalAstrometricFitConnections(


Cool...thanks for checking with the masses!

laurenam · 2024-01-20T20:47:56Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+
+        refConfig = LoadReferenceObjectsConfig()
+        refConfig.anyFilterMapsToThis = "phot_g_mean"
+        refConfig.requireProperMotion = True


But what if another survey wanted/needed to use something else? One use case I'm thinking of is LSSTCam-imSim...

laurenam · 2024-01-20T20:51:52Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+        Parameters
+        ----------
+        inputVisitSummaries : `list` of `lsst.afw.table.ExposureCatalog`
+            List of catalogs with per-detector summary information.


I think it may have changed in the past...this is "current" truth 😉

laurenam · 2024-01-20T20:59:12Z

python/lsst/drp/tasks/gbdesAstrometricFit.py

+
+        for i in range(clusters.n_clusters_):
+            fieldInd = clusters.labels_ == i
+            fieldDetectors = sum([allDetectorCorners[f] for f, fInd in enumerate(fieldInd) if fInd], [])


It's the sum part that is confusing me...but if it's doing what you want (and now describe), ok!

cmsaunders marked this pull request as ready for review November 21, 2023 17:30

cmsaunders force-pushed the tickets/DM-39776 branch from 15f73e3 to aa5f7bf Compare November 21, 2023 20:01

cmsaunders force-pushed the tickets/DM-39776 branch from fe9b68f to 33f75f2 Compare November 29, 2023 19:24

laurenam reviewed Dec 14, 2023

View reviewed changes

cmsaunders force-pushed the tickets/DM-39776 branch 2 times, most recently from ae5176c to d14f708 Compare December 15, 2023 17:21

laurenam reviewed Dec 27, 2023

View reviewed changes

laurenam reviewed Jan 12, 2024

View reviewed changes

cmsaunders force-pushed the tickets/DM-39776 branch from d14f708 to 73a0376 Compare January 19, 2024 17:05

laurenam approved these changes Jan 20, 2024

View reviewed changes

cmsaunders force-pushed the tickets/DM-39776 branch from a07c7a0 to 250a201 Compare January 22, 2024 22:12

cmsaunders force-pushed the tickets/DM-39776 branch 2 times, most recently from 06db50a to a107c60 Compare February 13, 2024 20:19

Add global gbdes task

d65fc8f

cmsaunders force-pushed the tickets/DM-39776 branch from a256bb0 to d65fc8f Compare February 14, 2024 02:44

cmsaunders merged commit 6ef0a89 into main Feb 14, 2024
3 checks passed

cmsaunders deleted the tickets/DM-39776 branch February 14, 2024 15:13

DM-39776: Add global astrometric fit task #30

DM-39776: Add global astrometric fit task #30

Conversation

cmsaunders commented Oct 17, 2023

arunkannawadi commented Oct 20, 2023

cmsaunders commented Oct 20, 2023

timj commented Oct 20, 2023

arunkannawadi commented Oct 20, 2023

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

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