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

Review for DM-3483: don't throw when transforming negative fluxes #24

Merged
merged 2 commits into from
Aug 19, 2015
Merged

Review for DM-3483: don't throw when transforming negative fluxes #24

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6af29e
Do not throw throw on transforming -ve flux to mag
jdswinbank Aug 18, 2015
8062ff7
Prefer check for inequality of iterators.
jdswinbank Aug 19, 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
15 changes: 15 additions & 0 deletions include/lsst/meas/base/FluxUtilities.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,21 @@ class FluxTransform : public BaseTransform {
MagResultKey _magKey;
};

/**
* Temporarily replace negative fluxes with NaNs
*
* Instantiating a NoThrowOnNegativeFluxContext object will cause afw::image::Calib
* objects to return NaN, rather than throwing, when asked to convert a negative flux to
* a magnitude for the lifetime of the NoThrowOnNegativeFluxContext.
*/
class NoThrowOnNegativeFluxContext {
public:
NoThrowOnNegativeFluxContext();
~NoThrowOnNegativeFluxContext();
private:
bool _throwOnNegative;
};

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 like this a lot. I'd like it even better if we could integrate it more closely with Calib, but I don't think that's appropriate for this ticket (it's more disruptive), so I've opened DM-3489 for that.

}}} // lsst::meas::base

#endif // !LSST_MEAS_BASE_FluxUtilities_h_INCLUDED
48 changes: 30 additions & 18 deletions python/lsst/meas/base/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,13 +590,14 @@ def tearDown(self):
del self.outputCat

def _populateCatalog(self, baseNames):
records = []
for flagValue in (True, False):
record = self.inputCat.addNew()
records.append(self.inputCat.addNew())
for baseName in baseNames:
self._setFieldsInRecord(record, baseName)
for flagName in self.flagNames:
if record.schema.join(baseName, flagName) in record.schema:
record.set(record.schema.join(baseName, flagName), flagValue)
if records[-1].schema.join(baseName, flagName) in records[-1].schema:
records[-1].set(records[-1].schema.join(baseName, flagName), flagValue)
self._setFieldsInRecords(records, baseName)

def _checkOutput(self, baseNames):
for inSrc, outSrc in zip(self.inputCat, self.outputCat):
Expand Down Expand Up @@ -692,26 +693,37 @@ def _setupTransform(self):


class FluxTransformTestCase(TransformTestCase):
def _setFieldsInRecord(self, record, name):
record[record.schema.join(name, 'flux')] = numpy.random.random()
record[record.schema.join(name, 'fluxSigma')] = numpy.random.random()
def _setFieldsInRecords(self, records, name):
for record in records:
record[record.schema.join(name, 'flux')] = numpy.random.random()
record[record.schema.join(name, 'fluxSigma')] = numpy.random.random()

# Negative fluxes should be converted to NaNs.
assert len(records) > 1
records[0][record.schema.join(name, 'flux')] = -1

def _compareFieldsInRecords(self, inSrc, outSrc, name):
fluxName, fluxSigmaName = inSrc.schema.join(name, 'flux'), inSrc.schema.join(name, 'fluxSigma')
mag, magErr = self.calexp.getCalib().getMagnitude(inSrc[fluxName], inSrc[fluxSigmaName])
self.assertEqual(outSrc[outSrc.schema.join(name, 'mag')], mag)
self.assertEqual(outSrc[outSrc.schema.join(name, 'magErr')], magErr)
if inSrc[fluxName] > 0:
mag, magErr = self.calexp.getCalib().getMagnitude(inSrc[fluxName], inSrc[fluxSigmaName])
self.assertEqual(outSrc[outSrc.schema.join(name, 'mag')], mag)
self.assertEqual(outSrc[outSrc.schema.join(name, 'magErr')], magErr)
else:
self.assertTrue(numpy.isnan(outSrc[outSrc.schema.join(name, 'mag')]))
self.assertTrue(numpy.isnan(outSrc[outSrc.schema.join(name, 'magErr')]))


class CentroidTransformTestCase(TransformTestCase):
def _setFieldsInRecord(self, record, name):
record[record.schema.join(name, 'x')], record[record.schema.join(name, 'y')] = numpy.random.random(2)
# Some algorithms set no errors; some set only sigma on x & y; some provide
# a full covariance matrix. Set only those which exist in the schema.
for fieldSuffix in ('xSigma', 'ySigma', 'x_y_Cov'):
fieldName = record.schema.join(name, fieldSuffix)
if fieldName in record.schema:
record[fieldName] = numpy.random.random()
def _setFieldsInRecords(self, records, name):
for record in records:
record[record.schema.join(name, 'x')] = numpy.random.random()
record[record.schema.join(name, 'y')] = numpy.random.random()
# Some algorithms set no errors; some set only sigma on x & y; some provide
# a full covariance matrix. Set only those which exist in the schema.
for fieldSuffix in ('xSigma', 'ySigma', 'x_y_Cov'):
fieldName = record.schema.join(name, fieldSuffix)
if fieldName in record.schema:
record[fieldName] = numpy.random.random()

def _compareFieldsInRecords(self, inSrc, outSrc, name):
centroidResultKey = CentroidResultKey(inSrc.schema[self.name])
Expand Down
13 changes: 9 additions & 4 deletions src/ApertureFlux.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,10 +354,15 @@ void ApertureFluxTransform::operator()(
}
afw::table::SourceCatalog::const_iterator inSrc = inputCatalog.begin();
afw::table::BaseCatalog::iterator outSrc = outputCatalog.begin();
for (; inSrc < inputCatalog.end() && outSrc < outputCatalog.end(); ++inSrc, ++outSrc) {
for (std::size_t i = 0; i < _ctrl.radii.size(); ++i) {
FluxResult fluxResult = fluxKeys[i].get(*inSrc);
_magKeys[i].set(*outSrc, calib.getMagnitude(fluxResult.flux, fluxResult.fluxSigma));
{
// While noThrow is in scope, converting a negative flux to a magnitude
// returns NaN rather than throwing.
NoThrowOnNegativeFluxContext noThrow;
for (; inSrc < inputCatalog.end() && outSrc < outputCatalog.end(); ++inSrc, ++outSrc) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This part of these lines isn't what's been changed, but...

  • I'm not sure using < on iterators is safe in general; it's certainly more idiomatic to use != on for loop exit conditions.
  • Iterator declarations seem a good place to use auto.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, to clarify, < is valid for RandomAccessIterators (which these are), but not for the more general InputIterator concept, so it's just bit more generic to use !=.

for (std::size_t i = 0; i < _ctrl.radii.size(); ++i) {
FluxResult fluxResult = fluxKeys[i].get(*inSrc);
_magKeys[i].set(*outSrc, calib.getMagnitude(fluxResult.flux, fluxResult.fluxSigma));
}
}
}
}
Expand Down
20 changes: 17 additions & 3 deletions src/FluxUtilities.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,24 @@ void FluxTransform::operator()(
FluxResultKey fluxKey(inputCatalog.getSchema()[_name]);
afw::table::SourceCatalog::const_iterator inSrc = inputCatalog.begin();
afw::table::BaseCatalog::iterator outSrc = outputCatalog.begin();
for (; inSrc < inputCatalog.end() && outSrc < outputCatalog.end(); ++inSrc, ++outSrc) {
FluxResult fluxResult = fluxKey.get(*inSrc);
_magKey.set(*outSrc, calib.getMagnitude(fluxResult.flux, fluxResult.fluxSigma));
{
// While noThrow is in scope, converting a negative flux to a magnitude
// returns NaN rather than throwing.
NoThrowOnNegativeFluxContext noThrow;
for (; inSrc < inputCatalog.end() && outSrc < outputCatalog.end(); ++inSrc, ++outSrc) {
FluxResult fluxResult = fluxKey.get(*inSrc);
_magKey.set(*outSrc, calib.getMagnitude(fluxResult.flux, fluxResult.fluxSigma));
}
}
}

NoThrowOnNegativeFluxContext::NoThrowOnNegativeFluxContext() {
_throwOnNegative = afw::image::Calib::getThrowOnNegativeFlux();
afw::image::Calib::setThrowOnNegativeFlux(false);
}

NoThrowOnNegativeFluxContext::~NoThrowOnNegativeFluxContext() {
afw::image::Calib::setThrowOnNegativeFlux(_throwOnNegative);
}

}}} // namespace lsst::meas::base
11 changes: 6 additions & 5 deletions tests/testSdssShape.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,12 @@ class SdssShapeTransformTestCase(FluxTransformTestCase, CentroidTransformTestCas
singleFramePlugins = ('base_SdssShape',)
forcedPlugins = ('base_SdssShape',)

def _setFieldsInRecord(self, record, name):
FluxTransformTestCase._setFieldsInRecord(self, record, name)
CentroidTransformTestCase._setFieldsInRecord(self, record, name)
for field in ('xx', 'yy', 'xy', 'xxSigma', 'yySigma', 'xySigma'):
record[record.schema.join(name, field)] = numpy.random.random()
def _setFieldsInRecords(self, records, name):
FluxTransformTestCase._setFieldsInRecords(self, records, name)
CentroidTransformTestCase._setFieldsInRecords(self, records, name)
for record in records:
for field in ('xx', 'yy', 'xy', 'xxSigma', 'yySigma', 'xySigma'):
record[record.schema.join(name, field)] = numpy.random.random()

def _compareFieldsInRecords(self, inSrc, outSrc, name):
FluxTransformTestCase._compareFieldsInRecords(self, inSrc, outSrc, name)
Expand Down