Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-3455

ProcessImageTask.matchSources fails if using ANetAstrometryTask

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_astrom
    • Labels:
      None
    • Story Points:
      1
    • Sprint:
      Science Pipelines DM-S15-6
    • Team:
      Alert Production

      Description

      ProcessImageTask.matchSources fails when using ANetAstrometryTask with the following error:

      processCcd.calibrate.astrometry: Applying distortion correction
      processCcd FATAL: Failed on dataId={'taiObs': '2013-11-03', 'pointing': 672, 'visit': 904400, 'dateObs': '2013-11-03', 'filter': 'HSC-Y', 'field': 'STRIPE82L', 'ccd': 50, 'expTime': 30.0}: 
        File "src/table/Schema.cc", line 239, in lsst::afw::table::SchemaItem<T> lsst::afw::table::detail::SchemaImpl::find(const string&) const [with T = double; std::string = std::basic_string<char>]
          Field or subfield withname 'astrom_distorted_x' not found with type 'D'. {0}
      lsst::pex::exceptions::NotFoundError: 'Field or subfield withname 'astrom_distorted_x' not found with type 'D'.'
       
      Traceback (most recent call last):
        File "/ssd/rowen/lsstsw/stack/Linux64/pipe_base/10.1-4-g6ba0cc7+3/python/lsst/pipe/base/cmdLineTask.py", line 320, in __call__
          result = task.run(dataRef, **kwargs)
        File "/ssd/rowen/lsstsw/stack/Linux64/pipe_base/10.1-4-g6ba0cc7+3/python/lsst/pipe/base/timer.py", line 118, in wrapper
          res = func(self, *args, **keyArgs)
        File "/ssd/rowen/lsstsw/stack/Linux64/pipe_tasks/tickets.DM-3453-g086c9ddd0a/python/lsst/pipe/tasks/processCcd.py", line 85, in run
          result = self.process(sensorRef, postIsrExposure)
        File "/ssd/rowen/lsstsw/stack/Linux64/pipe_base/10.1-4-g6ba0cc7+3/python/lsst/pipe/base/timer.py", line 118, in wrapper
          res = func(self, *args, **keyArgs)
        File "/ssd/rowen/lsstsw/stack/Linux64/pipe_tasks/tickets.DM-3453-g086c9ddd0a/python/lsst/pipe/tasks/processImage.py", line 219, in process
          srcMatches, srcMatchMeta = self.matchSources(calExposure, sources)
        File "/ssd/rowen/lsstsw/stack/Linux64/pipe_tasks/tickets.DM-3453-g086c9ddd0a/python/lsst/pipe/tasks/processImage.py", line 250, in matchSources
          astromRet = astrometry.loadAndMatch(exposure=exposure, sourceCat=sources)
        File "/ssd/rowen/lsstsw/stack/Linux64/pipe_base/10.1-4-g6ba0cc7+3/python/lsst/pipe/base/timer.py", line 118, in wrapper
          res = func(self, *args, **keyArgs)
        File "/ssd/rowen/lsstsw/stack/Linux64/meas_astrom/tickets.DM-3453-gbb2ad1f49c/python/lsst/meas/astrom/anetAstrometry.py", line 321, in loadAndMatch
          with self.distortionContext(sourceCat=sourceCat, exposure=exposure) as bbox:
        File "/ssd/rowen/lsstsw/anaconda/lib/python2.7/contextlib.py", line 17, in __enter__
          return self.gen.next()
        File "/ssd/rowen/lsstsw/stack/Linux64/meas_astrom/tickets.DM-3453-gbb2ad1f49c/python/lsst/meas/astrom/anetAstrometry.py", line 295, in distortionContext
          sourceCat.table.defineCentroid(self.distortedName)
        File "/ssd/rowen/lsstsw/stack/Linux64/afw/10.1-37-gaedf466/python/lsst/afw/table/tableLib.py", line 8887, in defineCentroid
          return _tableLib.SourceTable_defineCentroid(self, *args)
      NotFoundError: 
        File "src/table/Schema.cc", line 239, in lsst::afw::table::SchemaItem<T> lsst::afw::table::detail::SchemaImpl::find(const string&) const [with T = double; std::string = std::basic_string<char>]
          Field or subfield withname 'astrom_distorted_x' not found with type 'D'. {0}
      lsst::pex::exceptions::NotFoundError: 'Field or subfield withname 'astrom_distorted_x' not found with type 'D'.'
      

      This is probably a result of DM-2939. The basic problem is that the distortion context in ANetAstrometryTask should not be run at that point in processing. Paul Price suggests that a simple clean fix is to make the distortion context a no-op if the WCS already contains distortion, if that works. This is what I will try first.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            Paul Price's suggested fix worked perfectly. I confirmed it by doing two things:

            • Running HSC data with processCcd.py using ANetAstrometryTask
            • Forcing the distortion context to always be a no-op (by temporily hacking the new conditional) and verifying that everything ran. Accuracy may have suffered, but this proved that the distortion context can safely be disabled.
            Show
            rowen Russell Owen added a comment - Paul Price 's suggested fix worked perfectly. I confirmed it by doing two things: Running HSC data with processCcd.py using ANetAstrometryTask Forcing the distortion context to always be a no-op (by temporily hacking the new conditional) and verifying that everything ran. Accuracy may have suffered, but this proved that the distortion context can safely be disabled.
            Hide
            rowen Russell Owen added a comment -

            Work is on meas_astrom tickets/DM_3455. It is a very simple change to ANetAstrometryTask.

            Show
            rowen Russell Owen added a comment - Work is on meas_astrom tickets/DM_3455. It is a very simple change to ANetAstrometryTask.
            Hide
            jbosch Jim Bosch added a comment -

            Looks fine.

            One small comment: I was at first very confused by this entire context manager, because it looked like it was distorting the source positions in-place and then not undistorting them later, when it really puts the distorted source positions into a new field so there's nothing to undo besides the Wcs; the fact that the centroid distortion isn't an operation that needs to be undone is not at all apparent in the context manager itself, and it might be worth a comment. In the long term, I think it's probably bad for a context manager to be producing a side-effect like this, but I gather our hope is to ultimately remove ANetAstrometryTask anyway, and then the context manager will go away as well.

            Show
            jbosch Jim Bosch added a comment - Looks fine. One small comment: I was at first very confused by this entire context manager, because it looked like it was distorting the source positions in-place and then not undistorting them later, when it really puts the distorted source positions into a new field so there's nothing to undo besides the Wcs; the fact that the centroid distortion isn't an operation that needs to be undone is not at all apparent in the context manager itself, and it might be worth a comment. In the long term, I think it's probably bad for a context manager to be producing a side-effect like this, but I gather our hope is to ultimately remove ANetAstrometryTask anyway, and then the context manager will go away as well.
            Hide
            rowen Russell Owen added a comment - - edited

            Thank you for the review. I suspect ANetAstrometryTask could be rewritten to use a DistortedTanWcs instead of a distortion context. The code change would be simple but it would take significant work to prove that the results are no worse (if matching and fitting is done on sky then the results should be identical, but the work is done in x,y space then there are subtle differences). But as you say, I'm not sure if we'll be keeping this task around long enough to justify the effort.

            Show
            rowen Russell Owen added a comment - - edited Thank you for the review. I suspect ANetAstrometryTask could be rewritten to use a DistortedTanWcs instead of a distortion context. The code change would be simple but it would take significant work to prove that the results are no worse (if matching and fitting is done on sky then the results should be identical, but the work is done in x,y space then there are subtle differences). But as you say, I'm not sure if we'll be keeping this task around long enough to justify the effort.
            Hide
            price Paul Price added a comment -

            With a bit more thought, I'm not sure my suggested solution is sufficient in the general case. The problem is that the AstrometryTask being used to match sources was configured with a different schema (that from calibration) than it's given. So if the astrometric solution is linear (i.e., no distortion), then we're going to hit the same error, even with the fix.

            Show
            price Paul Price added a comment - With a bit more thought, I'm not sure my suggested solution is sufficient in the general case. The problem is that the AstrometryTask being used to match sources was configured with a different schema (that from calibration) than it's given. So if the astrometric solution is linear (i.e., no distortion), then we're going to hit the same error, even with the fix.
            Hide
            jbosch Jim Bosch added a comment -

            I think the real architectural problem is that ANetAstrometryTask needs to be able to use a column in its input catalog to store its temporaries. It's useful as diagnostic, of course, which is why we do it, but I think it's a bad side-effect from an interface standpoint and it prevents the class from being as reusable as it ought to be. There should probably be a way to call it in such a way that it just uses internal numpy arrays or something for the distorted coordinates rather than a new column (but note that this would have to be a calling option, not a config option, to solve this problem).

            But I think it'd be even better if the various astrometry tasks could return a more sophisticated match object that could be used to perform additional matches to the same reference catalog later. That would allow it to efficiently do a true N-way match without repeating any work already done in the first round, which is what we really want (since we also want to match calibration sources to regular sources in addition to matching both with the reference catalog. I think this part of the design is currently being hamstrung right now by our tendency to put all algorithmic Python code into Tasks (which I now think is maybe a bit overzealous), because Tasks are not allowed to have per-data-unit state (which a more efficient and intuitive match-multiple-times class would need to have).

            Show
            jbosch Jim Bosch added a comment - I think the real architectural problem is that ANetAstrometryTask needs to be able to use a column in its input catalog to store its temporaries. It's useful as diagnostic, of course, which is why we do it, but I think it's a bad side-effect from an interface standpoint and it prevents the class from being as reusable as it ought to be. There should probably be a way to call it in such a way that it just uses internal numpy arrays or something for the distorted coordinates rather than a new column (but note that this would have to be a calling option, not a config option, to solve this problem). But I think it'd be even better if the various astrometry tasks could return a more sophisticated match object that could be used to perform additional matches to the same reference catalog later. That would allow it to efficiently do a true N-way match without repeating any work already done in the first round, which is what we really want (since we also want to match calibration sources to regular sources in addition to matching both with the reference catalog. I think this part of the design is currently being hamstrung right now by our tendency to put all algorithmic Python code into Tasks (which I now think is maybe a bit overzealous), because Tasks are not allowed to have per-data-unit state (which a more efficient and intuitive match-multiple-times class would need to have).
            Hide
            rowen Russell Owen added a comment -

            I think the fix will get us through the AHM. I'm not sure what to suggest beyond that. I'll feel better about the new astrometry task once it matches in sky coordinates. Even then, it seems to be rather fragile – there are a extra loops that seem to be workarounds for underlying issues that I don't understand (and aren't in the original Tabur algorithm).

            Show
            rowen Russell Owen added a comment - I think the fix will get us through the AHM. I'm not sure what to suggest beyond that. I'll feel better about the new astrometry task once it matches in sky coordinates. Even then, it seems to be rather fragile – there are a extra loops that seem to be workarounds for underlying issues that I don't understand (and aren't in the original Tabur algorithm).

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Jim Bosch
                Watchers:
                Jim Bosch, Paul Price, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel