implement PhotoCalib tickets/DM-9192 #195

parejkoj · 2017-03-18T00:29:30Z

No description provided.

kfindeisen · 2017-03-30T22:21:29Z

include/lsst/afw/image/PhotoCalib.h

+     *
+     * @return     The flux in maggies and error (sigma).
+     */
+    std::pair<double, double> countsToMaggies(double counts, double countsSigma,


Is there a "value with error" class? Pair is really not the best API for this.

I won't ask if you really need eight overloads...

Pair is basically a python tuple containing two things, right? Other suggestions are welcome.

I'd be happy to flatten the overloads, if someone has a suggestion of how. About half of these are already used somewhere in the stack, the others (e.g. SourceRecord or SourceCatalog) are ones we know we want to use.

Pair is a bit generic, and it can be hard to remember what the first and second elements are after it's been passed around some. But the appropriateness/inappropriateness of tuples as return types is a controversial subject, so I won't belabor the point.

I'll have to think about the overloads; part of the problem is that apart from Point vs. average I don't see much of a pattern to them.

Maybe when I've got some more of the unittest written the use cases will be clearer?

jdswinbank · 2017-04-12T21:28:39Z

src/image/PhotoCalib.cc

+        return instance;
+    }
+
+    PersistenceHelper(int tableVersion=PHOTOCALIB_TABLE_CURRENT_VERSION) :


I realise the tableVersion is just here to provide for future evolution, but I'm unclear how it's supposed to work, in particular in relation to the get() above with its static instance.

(Per my comment on JIRA) could this be private?

Table version has been removed after discussion with @TallJimbo who said that we typically only do that when it becomes necessary. That may not be optimal, but then I can also wait and see if we implement a more persistence versioning generic system in the future.

jdswinbank · 2017-04-12T21:32:16Z

src/image/PhotoCalib.cc

+        field = schema.addField<int>("field", "archive ID of the BoundedField object");
+    }
+
+    PersistenceHelper(table::Schema const & s) :


Is this necessary? Could you just use PersistenceHelper::get()? Is the idea to support future evolution? If so, could you use get() in conjunction with a tableVersion derived from the table you're reading?

I mostly stole all of this code from @r-owen 's VisitInfo implementation. I don't know what best practices for persistence are, at all. Do we have any guide on the topic?

The most useful document I found was https://github.com/lsst/afw/blob/master/doc/tablePersistence.dox, which (for reasons I've not had chance to investigate) doesn't actually seem to be being published by Doxygen. It's by no means a complete reference though.

@jdswinbank it's published at https://lsst-web.ncsa.illinois.edu/doxygen/x_masterDoxyDoc/afw_table_persistence.html. Maybe a cross-reference needs to be added from the persistence classes?

I switched to using the get(), removed the schema-based constructor, and made the empty constructor private.

jdswinbank · 2017-04-12T22:50:13Z

src/math/ChebyshevBoundedField.cc

@@ -381,7 +381,7 @@ class ChebyshevBoundedFieldFactory : public table::io::PersistableFactory {
        LSST_ARCHIVE_ASSERT(catalogs.front().size() == 1u);
        table::BaseRecord const & record = catalogs.front().front();
        PersistenceHelper const keys(record.getSchema());
-        geom::Box2I bbox(record.get(keys.bboxMin), record.get(keys.bboxMax));
+        geom::Box2I bbox(record.get(keys.bboxMin), record.get(keys.bboxMax), false);


This is (I assume) what the commit message refers to as

Fix to ChebyshevBF when persisting empty BBox, with test

I think it'd be nice to add a note explaining what the issue was (to the code and/or the commit message), and citing RFC-324.

(Nice catch, by the way)

jdswinbank · 2017-04-12T22:53:00Z

src/math/ChebyshevBoundedField.cc

+        auto& rhsCasted = dynamic_cast<ChebyshevBoundedField const&>(rhs);
+        bool coefficientsEqual = _coefficients.getShape() == rhsCasted._coefficients.getShape() &&
+                                  all(equal(_coefficients, rhsCasted._coefficients));
+        return getBBox() == rhsCasted.getBBox() && coefficientsEqual;


Personally, I'd find that easier to read with some parentheses. I realise that the coding standards might advocate for omitting them, but here I think it's worth the extra visual noise. Your call.

jdswinbank · 2017-04-12T23:13:57Z

tests/testChebyshevBoundedField.py

@@ -62,6 +62,7 @@
 class ChebyshevBoundedFieldTestCase(lsst.utils.tests.TestCase):

    def setUp(self):
+        self.longMessage = True


I hadn't come across this before. Nice.

jdswinbank · 2017-04-13T21:02:32Z

src/image/PhotoCalib.cc

+        countsMag0 = schema.addField<double>("countsMag0", "counts of a zero-magnitude object", "count");
+        countsMag0Sigma = schema.addField<double>("countsMag0Sigma", "1-sigma error on countsmag0", "count");
+        isConstant = schema.addField<table::Flag>("isConstant", "Is this spatially-constant?");
+        field = schema.addField<int>("field", "archive ID of the BoundedField object");


Put these in the member initializer list?

Again per JIRA, you need to mark the schema as persistent.

jdswinbank · 2017-04-13T21:15:11Z

src/image/PhotoCalib.cc

+        (*iter)[1] = toMaggiesSigma(counts, countsSigma, countsMag0, _countsMag0Sigma, maggies);
+        iter++;
+    }
+    if (negatives > 0  && throwOnNegativeFlux) {


Hmm.

If you are returning a new array (ie, result was created by the immediate caller rather than being passed into it), and throwOnNegativeFlux is true, then after you've thrown nothing is going to get returned. You might as well save time by bailing after the first negative.

If you are not returning a new array, you're going to fill the complete array (with NaNs where you had negative counts), and then throw. The owner of the array will get back the data, but also have to deal with the exception. That's... not unreasonable, but probably deserves documenting.

Per our discussion, I've completely removed the throwOnNegative functionality, thus mooting this question.

jdswinbank · 2017-04-13T21:16:23Z

src/image/PhotoCalib.cc

+        (*iter)[1] = toMagnitudeSigma(counts, countsSigma, countsMag0, _countsMag0Sigma, maggies);
+        iter++;
+    }
+    if (negatives > 0  && throwOnNegativeFlux) {


Same comment as above, obviously.

throw functionality removed, so moot.

jdswinbank · 2017-04-13T22:55:58Z

python/lsst/afw/image/photoCalib.cc

+             "sourceCatalog"_a, "countsField"_a, "result"_a, "throwOnNegativeCounts"_a=false);
+
+    cls.def("countsToMaggies",
+            (void (PhotoCalib::*)(afw::table::SourceCatalog &, std::string const &, std::string const &, bool) const)


Line length.

jdswinbank · 2017-04-13T23:12:41Z

include/lsst/afw/image/PhotoCalib.h

+ *
+ * A PhotoCalib is a BoundedField (a function with a specified domain) that converts between calibrated
+ * counts-on-chip (ADU) to flux and magnitude. It is defined in terms of "maggies", which are a linear
+ * unit defined in SDSS: http://www.sdss.org/dr12/algorithms/magnitudes/#nmgy


maggies or nanomaggies?

I've defined PhotoCalib in terms of Maggies. I've clarified the docstring here to note that the SDSS link gives the definition in terms of nanomaggies.

parejkoj · 2017-04-21T21:08:44Z

I've renamed things to use "err" and also renamed counts to instFlux.

kfindeisen · 2017-04-28T17:27:15Z

src/math/ChebyshevBoundedField.cc

-        return false;
-    }
+    auto rhsCasted = dynamic_cast<ChebyshevBoundedField const *>(&rhs);
+    if (!rhsCasted) return false;


Is the "efficiency" just sidestepping the exception handler, or is there something more subtle going on?

It's just avoiding (possible) overhead from catch/throw. I'm not sure if the compiler should be able to optimize any of that away in this case, but I'm pretty sure some major ones wouldn't, at least as of a couple of years ago.

Add BoundedField str/repr to pybind11

Fix to ChebyshevBF when persisting empty BBox, with test Add note about invert bbox on ingest

RFC-322-related code not implemented Add ticket links and make isConstant a Flag

parejkoj force-pushed the tickets/DM-9192 branch from 67018d3 to ddd6a95 Compare March 21, 2017 04:30

parejkoj force-pushed the tickets/DM-9192 branch 2 times, most recently from 59d1b70 to 4f8fbb8 Compare March 30, 2017 18:29

kfindeisen reviewed Mar 30, 2017

View reviewed changes

parejkoj force-pushed the tickets/DM-9192 branch 6 times, most recently from 70b9062 to 862e6bb Compare April 8, 2017 23:18

jdswinbank reviewed Apr 13, 2017

View reviewed changes

parejkoj force-pushed the tickets/DM-9192 branch 5 times, most recently from d8e25cf to f13db1e Compare April 28, 2017 00:37

kfindeisen reviewed Apr 28, 2017

View reviewed changes

parejkoj force-pushed the tickets/DM-9192 branch from 5bf5ebf to daed811 Compare April 28, 2017 17:44

parejkoj added 3 commits April 28, 2017 10:50

Add basic operator<< for BoundedField and ChebyshevBF

7f49a56

Add BoundedField str/repr to pybind11

Add equality operator for BoundedFields

40aca26

Fix to ChebyshevBF when persisting empty BBox, with test Add note about invert bbox on ingest

PhotoCalib implemented

4a9eb9b

RFC-322-related code not implemented Add ticket links and make isConstant a Flag

parejkoj force-pushed the tickets/DM-9192 branch from daed811 to 4a9eb9b Compare April 28, 2017 17:57

parejkoj merged commit 4a9eb9b into master Apr 28, 2017

ktlim deleted the tickets/DM-9192 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement PhotoCalib tickets/DM-9192 #195

implement PhotoCalib tickets/DM-9192 #195

parejkoj commented Mar 18, 2017

kfindeisen Mar 30, 2017

parejkoj Mar 30, 2017

kfindeisen Mar 30, 2017

parejkoj Mar 30, 2017

jdswinbank Apr 12, 2017

jdswinbank Apr 12, 2017

parejkoj Apr 26, 2017

jdswinbank Apr 12, 2017

parejkoj Apr 19, 2017

jdswinbank Apr 19, 2017

kfindeisen Apr 19, 2017

jdswinbank Apr 19, 2017

parejkoj Apr 26, 2017

jdswinbank Apr 12, 2017

jdswinbank Apr 12, 2017

jdswinbank Apr 12, 2017

jdswinbank Apr 12, 2017

jdswinbank Apr 13, 2017

jdswinbank Apr 13, 2017

jdswinbank Apr 13, 2017

parejkoj Apr 18, 2017

jdswinbank Apr 13, 2017

parejkoj Apr 18, 2017

jdswinbank Apr 13, 2017

jdswinbank Apr 13, 2017

parejkoj Apr 18, 2017

parejkoj commented Apr 21, 2017

kfindeisen Apr 28, 2017

TallJimbo Apr 28, 2017

implement PhotoCalib tickets/DM-9192 #195

implement PhotoCalib tickets/DM-9192 #195

Conversation

parejkoj commented Mar 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment