Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tickets/DM-2931: truncation in Wcs persistence #30

Merged
merged 4 commits into from
Jun 23, 2015
Merged

tickets/DM-2931: truncation in Wcs persistence #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
76314ce
Make Wcs non-persistable, but TanWcs persistable
RobertLuptonTheGood Jun 22, 2015
80e607d
We construct a TAN WCS, so it needs to be TanWcs
RobertLuptonTheGood Jun 22, 2015
d573fed
If we cannot use the table-based Wcs fall back on the one in the header
RobertLuptonTheGood Jun 22, 2015
8cda15e
Added test for round-tripping non-persistable (ZPN) WCS
RobertLuptonTheGood Jun 23, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/lsst/afw/image/TanWcs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@ class TanWcs : public afw::table::io::PersistableFacade<TanWcs>, public lsst::af
Eigen::MatrixXd const & sipBp
);

/// @brief Whether the object is persistable using afw::table::io archives.
virtual bool isPersistable() const;

protected:

TanWcs(TanWcs const & rhs);
Expand Down
3 changes: 2 additions & 1 deletion include/lsst/afw/image/Wcs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,8 @@ class Wcs : public lsst::daf::base::Persistable,
protected:

friend class WcsFactory;

/// Perform basic checks on whether *this might be persistable
bool _mayBePersistable() const;
// See afw::table::io::Persistable
virtual std::string getPersistenceName() const;
virtual std::string getPythonModule() const;
Expand Down
16 changes: 12 additions & 4 deletions src/image/ExposureInfo.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,11 +254,19 @@ void ExposureInfo::_readFits(
}
int wcsId = popInt(*metadata, "WCS_ID");
try {
_wcs = archive.get<Wcs>(wcsId);
auto archiveWcs = archive.get<Wcs>(wcsId);
if (archiveWcs) {
_wcs = archiveWcs;
} else {
pex::logging::Log::getDefaultLog().info("Empty WCS extension, using FITS header");
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the warning message below this block is incorrect: we should say we're just using the Wcs we got from the header.

By the way, I just refreshed my memory on when that warning is supposed to appear: it's when we couldn't load the object because it's defined in an extension package that's not setup. If the WCS wasn't saved in the tables at all, we'll just get a null pointer back, and the block above is in play. If the WCS was saved but it's been corrupted, we'll get an exception that propagates all the way up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the comment

I also note that we could check for wcsId == 0, but doing that would break the symmetry with e.g. Psf handing and I didn't want to fix all of them on this ticket.

} catch (pex::exceptions::NotFoundError & err) {
pex::logging::Log::getDefaultLog().warn(
boost::format("Could not read WCS; setting to null: %s") % err.what()
);
auto msg = str(boost::format("Could not read WCS extension; setting to null: %s") % err.what());
if (_wcs) {
msg += " ; using WCS from FITS header";
}

pex::logging::Log::getDefaultLog().warn(msg);
}
int coaddInputsId = popInt(*metadata, "COADD_INPUTS_ID");
try {
Expand Down
8 changes: 8 additions & 0 deletions src/image/TanWcs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ TanWcs::TanWcs() :
_sipA(), _sipB(), _sipAp(), _sipBp()
{}

bool TanWcs::isPersistable() const {
if (!_mayBePersistable()) {
return false;
}

return true;
}

geom::Angle TanWcs::pixelScale() const {
// HACK -- assume "CD" elements are set (and are in degrees)
double* cd = _wcsInfo->m_cd;
Expand Down
11 changes: 10 additions & 1 deletion src/image/Wcs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1102,13 +1102,22 @@ void Wcs::write(OutputArchiveHandle & handle) const {
handle.saveCatalog(catalog);
}

bool Wcs::isPersistable() const {
bool Wcs::_mayBePersistable() const {
if (_wcsInfo[0].naxis != 2) return false;
if (std::strcmp(_wcsInfo[0].cunit[0], "deg") != 0) return false;
if (std::strcmp(_wcsInfo[0].cunit[1], "deg") != 0) return false;

return true;
}

bool Wcs::isPersistable() const {
if (!_mayBePersistable()) {
return false;
}
// The current table persistence only works for TAN and TAN-SIP projections
return false;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to call _mayBePersistable() at all here, since we're going to return false regardless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote it that way as a template for derived classes. It doesn't cost enough to matter.

Wcs::Wcs(afw::table::BaseRecord const & record) :
daf::base::Citizen(typeid(this)),
_wcsInfo(NULL),
Expand Down
2 changes: 1 addition & 1 deletion src/image/makeWcs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,5 +146,5 @@ afwImg::Wcs::Ptr afwImg::makeWcs(
lsst::afw::geom::Point2D crvalTmp;
crvalTmp[0] = crval.toIcrs().getLongitude().asDegrees();
crvalTmp[1] = crval.toIcrs().getLatitude().asDegrees();
return afwImg::Wcs::Ptr(new lsst::afw::image::Wcs(crvalTmp, crpix, CD));
return afwImg::Wcs::Ptr(new lsst::afw::image::TanWcs(crvalTmp, crpix, CD));
}
Binary file added tests/data/ZPN.fits
View file
Open in desktop
Binary file not shown.
14 changes: 14 additions & 0 deletions tests/testWcsFitsTable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,20 @@ def testExposure(self):
wcsOut = expOut.getWcs()
self.assertEqual(wcsIn, wcsOut)

def testWcsWhenNonPersistable(self):
"""Test that we can round-trip a WCS even when it is not persistable"""
import os

fileName = os.path.join(os.path.split(__file__)[0], "data", "ZPN.fits")
exp = lsst.afw.image.ExposureF(fileName)
del fileName

self.assertFalse(exp.getWcs().isPersistable(), "Test assumes that ZPN projections are not persistable")

with utilsTests.getTempFilePath(".fits") as fileName:
exp.writeFits(fileName)
exp2 = lsst.afw.image.ExposureF(fileName)
self.assertEqual(exp.getWcs(), exp2.getWcs())
#####

def suite():
Expand Down