XMLWordPrintable

Details

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

Description

 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 lsst::afw::table::detail::SchemaImpl::find(const string&) const [with T = double; std::string = std::basic_string]  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 lsst::afw::table::detail::SchemaImpl::find(const string&) const [with T = double; std::string = std::basic_string]  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.

Activity

Hide
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
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
Russell Owen added a comment -

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

Show
Russell Owen added a comment - Work is on meas_astrom tickets/DM_3455. It is a very simple change to ANetAstrometryTask.
Hide
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
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
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
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
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
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
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
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
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
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:
Russell Owen
Reporter:
Russell Owen
Reviewers:
Jim Bosch
Watchers:
Jim Bosch, Paul Price, Russell Owen