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.

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

DM-34484: Update tests to remove dependency on obs_test. #278

Merged
merged 8 commits into from
May 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
45 changes: 33 additions & 12 deletions python/lsst/meas/algorithms/loadReferenceObjects.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ def convertToNanojansky(catalog, log, doConvert=True):
if doConvert:
newSchema = mapper.getOutputSchema()
output = afwTable.SimpleCatalog(newSchema)
output.reserve(len(catalog))
output.extend(catalog, mapper=mapper)
for field in output_fields:
output[field.getName()] *= 1e9
Expand Down Expand Up @@ -758,14 +759,16 @@ class ReferenceObjectLoader(ReferenceObjectLoaderBase):
dataIds : iterable of `lsst.daf.butler.DataCoordinate`
An iterable object of data IDs that point to reference catalogs
in a gen 3 repository.
refCats : iterable of `lsst.daf.butler.DeferedDatasetHandle`
refCats : iterable of `lsst.daf.butler.DeferredDatasetHandle`
Handles to load refCats on demand
log : `lsst.log.Log`, `logging.Logger` or `None`, optional
Logger object used to write out messages. If `None` a default
logger will be used.
"""
def __init__(self, dataIds, refCats, log=None, **kwargs):
super().__init__(**kwargs)
def __init__(self, dataIds, refCats, log=None, config=None, **kwargs):
if config is None:
config = self.ConfigClass()
super().__init__(config=config, **kwargs)
self.dataIds = dataIds
self.refCats = refCats
self.log = log or logging.getLogger(__name__).getChild("ReferenceObjectLoader")
Expand Down Expand Up @@ -805,9 +808,15 @@ def loadPixelBox(self, bbox, wcs, filterName, epoch=None,

Returns
-------
referenceCatalog : `lsst.afw.table.SimpleCatalog`
Catalog containing reference objects inside the specified bounding
box (padded by self.config.pixelMargin).
output : `lsst.pipe.base.Struct`
Results struct with attributes:

``refCat``
Catalog containing reference objects inside the specified
bounding box (padded by self.config.pixelMargin).
``fluxField``
Name of the field containing the flux associated with
``filterName``.

Raises
------
Expand Down Expand Up @@ -886,9 +895,15 @@ class which intersect or are contained within the specified region. The

Returns
-------
referenceCatalog : `lsst.afw.table.SourceCatalog`
Catalog containing reference objects which intersect the input region,
filtered by the specified filter function.
output : `lsst.pipe.base.Struct`
Results struct with attributes:

``refCat``
Catalog containing reference objects which intersect the
input region, filtered by the specified filter function.
``fluxField``
Name of the field containing the flux associated with
``filterName``.

Raises
------
Expand Down Expand Up @@ -991,9 +1006,15 @@ def loadSkyCircle(self, ctrCoord, radius, filterName, epoch=None):

Returns
-------
referenceCatalog : `lsst.afw.table.SourceCatalog`
Catalog containing reference objects inside the specified search
circle.
output : `lsst.pipe.base.Struct`
Results struct with attributes:

``refCat``
Catalog containing reference objects inside the specified
search circle.
``fluxField``
Name of the field containing the flux associated with
``filterName``.
"""
centerVector = ctrCoord.getVector()
sphRadius = sphgeom.Angle(radius.asRadians())
Expand Down
134 changes: 133 additions & 1 deletion python/lsst/meas/algorithms/testUtils.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,21 @@
# see <https://www.lsstcorp.org/LegalNotices/>.
#

__all__ = ["plantSources", "makeRandomTransmissionCurve", "makeDefectList"]
__all__ = ["plantSources", "makeRandomTransmissionCurve", "makeDefectList",
"MockReferenceObjectLoaderFromFiles"]

import numpy as np
import esutil

import lsst.geom
import lsst.afw.image as afwImage
from lsst import sphgeom
from . import SingleGaussianPsf
from . import Defect

from . import ReferenceObjectLoader
import lsst.afw.table as afwTable


def plantSources(bbox, kwid, sky, coordList, addPoissonNoise=True):
"""Make an exposure with stars (modelled as Gaussians)
Expand Down Expand Up @@ -178,3 +184,129 @@ def makeDefectList():
]

return defectList


class MockRefcatDataId:
"""Mock reference catalog dataId.

The reference catalog dataId is only used to retrieve a region property.

Parameters
----------
pixelization : `lsst.sphgeom.Pixelization`
Pixelization object.
index : `int`
Pixel index number.
"""
def __init__(self, pixelization, index):
self._region = pixelization.pixel(index)

@property
def region(self):
return self._region


class MockRefcatDeferredDatasetHandle:
"""Mock reference catalog dataset handle.

The mock handle needs a get() and a name for logging.

Parameters
----------
catalog : `lsst.afw.table.SourceCatalog`
Reference catalog.
name : `str`
Name of reference catalog.
"""
def __init__(self, catalog, name):
self._catalog = catalog
self._name = name

def get(self):
return self._catalog

class MockRef:
def __init__(self, name):
self._name = name

class MockDatasetType:
def __init__(self, name):
self._name = name

@property
def name(self):
return self._name

@property
def datasetType(self):
return self.MockDatasetType(self._name)

@property
def ref(self):
return self.MockRef(self._name)


class MockReferenceObjectLoaderFromFiles(ReferenceObjectLoader):
"""A simple mock of ReferenceObjectLoader.

This mock ReferenceObjectLoader uses a set of files on disk to create
mock dataIds and data reference handles that can be accessed
without a butler. The files must be afw catalog files in the reference
catalog format, sharded with HTM pixelization.

Parameters
----------
filenames : `list` [`str`]
Names of files to use.
config : `lsst.meas.astrom.LoadReferenceObjectsConfig`, optional
Configuration object if necessary to override defaults.
htmLevel : `int`, optional
HTM level to use for the loader.
"""
def __init__(self, filenames, name='cal_ref_cat', config=None, htmLevel=4):
dataIds, refCats = self._createDataIdsAndRefcats(filenames, htmLevel, name)

super().__init__(dataIds, refCats, config=config)

def _createDataIdsAndRefcats(self, filenames, htmLevel, name):
"""Create mock dataIds and refcat handles.

Parameters
----------
filenames : `list` [`str`]
Names of files to use.
htmLevel : `int`
HTM level to use for the loader.
name : `str`
Name of reference catalog (for logging).

Returns
-------
dataIds : `list` [`MockRefcatDataId`]
List of mock dataIds.
refCats : `list` [`MockRefcatDeferredDatasetHandle`]
List of mock deferred dataset handles.

Raises
------
RuntimeError if any file contains sources that cover more than one HTM
pixel at level ``htmLevel``.
"""
pixelization = sphgeom.HtmPixelization(htmLevel)
htm = esutil.htm.HTM(htmLevel)

dataIds = []
refCats = []

for filename in filenames:
cat = afwTable.BaseCatalog.readFits(filename)

ids = htm.lookup_id(np.rad2deg(cat['coord_ra']), np.rad2deg(cat['coord_dec']))

if len(np.unique(ids)) != 1:
raise RuntimeError(f"File {filename} contains more than one pixel at level {htmLevel}")
erykoff marked this conversation as resolved.
Show resolved Hide resolved

dataIds.append(MockRefcatDataId(pixelization, ids[0]))
refCats.append(MockRefcatDeferredDatasetHandle(cat, name))

return dataIds, refCats
1 change: 0 additions & 1 deletion tests/data/version0/_mapper

This file was deleted.

1 change: 0 additions & 1 deletion tests/data/version1/_mapper

This file was deleted.

12 changes: 6 additions & 6 deletions tests/ingestIndexTestBase.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

import logging
import math
import shutil
import string
import tempfile

Expand Down Expand Up @@ -178,10 +177,7 @@ def get_word(word_len):

@classmethod
def tearDownClass(cls):
try:
shutil.rmtree(cls.outPath)
except Exception:
print("WARNING: failed to remove temporary dir %r" % (cls.outPath,))
cls.outDir.cleanup()
del cls.outPath
del cls.skyCatalogFile
del cls.skyCatalogFileDelim
Expand All @@ -193,7 +189,8 @@ def tearDownClass(cls):

@classmethod
def setUpClass(cls):
cls.outPath = tempfile.mkdtemp()
cls.outDir = tempfile.TemporaryDirectory()
Copy link
Member

Choose a reason for hiding this comment

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

We try to avoid using this in tests because it can lead to test failures over NFS caused by TemporaryDirectory not using the ignore_errors flag in shutil when it automatically runs the cleanup (because of nfs lock files hanging around). You might get away with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no! Okay, we had a mix before and I thought that mkdtemp was old and TemporaryDirectory was new. I can move everything over to mkdtemp. The problem I was trying to solve was that the temporary directories aren't being cleaned up. (These might be fine because they all go in /tmp and who mounts /tmp over nfs?)

Copy link
Member

@TallJimbo TallJimbo Apr 25, 2022

Choose a reason for hiding this comment

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

Other people may be excited about fancy case syntax, but ignore_cleanup_errors is what I'm really looking forward to in Python 3.10.

Copy link
Member

Choose a reason for hiding this comment

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

Ooh. They've finally fixed it to allow you to say you don't care. If only someone would write a 3.10 migration RFC.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Don't we have lsst.utils.tests.temporaryDirectory that tries to fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: should I move this all back to mkdtemp and add the robust cleanup now, or let this slide until we get to 3.10? I think it's fine because /tmp shouldn't be nfs mounted, but that doesn't mean it can't be nfs mounted.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to lsst.utils.tests.temporaryDirectory as the solution for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm. That's a context manager so does it work against setup/teardown which we need here?

Copy link
Member

Choose a reason for hiding this comment

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

🤔 You are probably right that /tmp is not going to mess us up. I think the problem we had before was from people creating temp directories within the tests/ directory (which is still common).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any notes about temporary directory usage in the dev guide, and I wasn't aware of that problem. Should we add a section about that to the python testing guide?

cls.outPath = cls.outDir.name
# arbitrary, but reasonable, amount of proper motion (angle/year)
# and direction of proper motion
cls.properMotionAmt = 3.0*lsst.geom.arcseconds
Expand Down Expand Up @@ -227,6 +224,9 @@ def setUp(self):
self.butler = self.makeTemporaryRepo(self.repoPath.name, self.depth)
self.logger = logging.getLogger('lsst.ReferenceObjectLoader')

def tearDown(self):
self.repoPath.cleanup()

@staticmethod
def makeTemporaryRepo(rootPath, depth):
"""Create a temporary butler repository, configured to support a given
Expand Down
25 changes: 19 additions & 6 deletions tests/nopytest_ingestIndexReferenceCatalog.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,11 @@ class TestConvertReferenceCatalogParallel(ingestIndexTestBase.ConvertReferenceCa
def testIngestTwoFilesTwoCores(self):
def runTest(withRaDecErr):
# Generate a second catalog, with different ids
inPath1 = tempfile.mkdtemp()
inTempDir1 = tempfile.TemporaryDirectory()
inPath1 = inTempDir1.name
skyCatalogFile1, _, skyCatalog1 = self.makeSkyCatalog(inPath1, idStart=25, seed=123)
inPath2 = tempfile.mkdtemp()
inTempDir2 = tempfile.TemporaryDirectory()
inPath2 = inTempDir2.name
skyCatalogFile2, _, skyCatalog2 = self.makeSkyCatalog(inPath2, idStart=5432, seed=11)
# override some field names, and use multiple cores
config = ingestIndexTestBase.makeConvertConfig(withRaDecErr=withRaDecErr, withMagErr=True,
Expand All @@ -71,7 +73,8 @@ def runTest(withRaDecErr):
"_withRaDecErr" if withRaDecErr else "_noRaDecErr")

# Convert the input data files to our HTM indexed format.
dataPath = tempfile.mkdtemp()
dataTempDir = tempfile.TemporaryDirectory()
dataPath = dataTempDir.name
converter = ConvertReferenceCatalogTask(output_dir=dataPath, config=config)
converter.run([skyCatalogFile1, skyCatalogFile2])

Expand Down Expand Up @@ -109,6 +112,10 @@ def runTest(withRaDecErr):
self.checkAllRowsInRefcat(loader, skyCatalog1, config)
self.checkAllRowsInRefcat(loader, skyCatalog2, config)

inTempDir1.cleanup()
inTempDir2.cleanup()
dataTempDir.cleanup()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just remove all the calls to cleanup in this branch.


runTest(withRaDecErr=True)
runTest(withRaDecErr=False)

Expand All @@ -122,7 +129,8 @@ class TestConvertRefcatManager(ingestIndexTestBase.ConvertReferenceCatalogTestBa
def setUp(self):
np.random.seed(10)

tempPath = tempfile.mkdtemp()
self.tempDir = tempfile.TemporaryDirectory()
tempPath = self.tempDir.name
self.log = lsst.log.Log.getLogger("lsst.TestIngestIndexManager")
self.config = ingestIndexTestBase.makeConvertConfig(withRaDecErr=True)
self.config.id_name = 'id'
Expand All @@ -137,8 +145,9 @@ def setUp(self):

self.fakeInput = self.makeSkyCatalog(outPath=None, size=5, idStart=6543)
self.matchedPixels = np.array([1, 1, 2, 2, 3])
self.path = tempfile.mkdtemp()
self.filenames = {x: os.path.join(self.path, "%d.fits" % x) for x in set(self.matchedPixels)}
self.tempDir2 = tempfile.TemporaryDirectory()
tempPath = self.tempDir2.name
self.filenames = {x: os.path.join(tempPath, "%d.fits" % x) for x in set(self.matchedPixels)}

self.worker = ConvertRefcatManager(self.filenames,
self.config,
Expand Down Expand Up @@ -233,6 +242,10 @@ def test_getCatalog(self):
self.assertFloatsEqual(newcat[:len(catalog)]['coord_ra'], catalog['coord_ra'])
self.assertFloatsEqual(newcat[:len(catalog)]['coord_dec'], catalog['coord_dec'])

def tearDown(self):
self.tempDir.cleanup()
self.tempDir2.cleanup()
Copy link
Contributor

Choose a reason for hiding this comment

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

More unnecessary cleanups



class TestMemory(lsst.utils.tests.MemoryTestCase):
pass
Expand Down