Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-9192

Implement new spatially-variable PhotoCalib model

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      16
    • Sprint:
      Alert Production S17 - 2, Alert Production S17 - 3, Alert Production S17 - 4
    • Team:
      Alert Production

      Description

      This ticket is for the implementation of the BoundedField-based Calib redesign that will be proposed in an upcoming RFC. The details of the design will be given below once they are settled.

        Attachments

          Issue Links

            Activity

            Parejkoj John Parejko created issue -
            Parejkoj John Parejko made changes -
            Field Original Value New Value
            Epic Link DM-9184 [ 29766 ]
            Parejkoj John Parejko made changes -
            Link This issue is triggering DM-9195 [ DM-9195 ]
            Parejkoj John Parejko made changes -
            Link This issue is blocked by DM-9193 [ DM-9193 ]
            krughoff Simon Krughoff made changes -
            Sprint Alert Production S17 - 2 [ 361 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            Parejkoj John Parejko made changes -
            Rank Ranked lower
            krughoff Simon Krughoff made changes -
            Sprint Alert Production S17 - 2 [ 361 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            Parejkoj John Parejko made changes -
            Summary Implement new Calib model post-RFC Implement new spatially-variable PhotoCalib model
            Parejkoj John Parejko made changes -
            Link This issue is triggered by RFC-289 [ RFC-289 ]
            Hide
            Parejkoj John Parejko added a comment -

            Beginning to implement the design described in RFC-289.

            Show
            Parejkoj John Parejko added a comment - Beginning to implement the design described in RFC-289 .
            Parejkoj John Parejko made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            krughoff Simon Krughoff made changes -
            Sprint Alert Production S17 - 2 [ 361 ] Alert Production S17 - 2, Alert Production S17 - 3 [ 361, 605 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            Parejkoj John Parejko made changes -
            Link This issue is blocked by DM-9882 [ DM-9882 ]
            krughoff Simon Krughoff made changes -
            Sprint Alert Production S17 - 2, Alert Production S17 - 3 [ 361, 605 ] Alert Production S17 - 2, Alert Production S17 - 3, Alert Production S17 - 4 [ 361, 605, 610 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            Parejkoj John Parejko made changes -
            Link This issue relates to RFC-322 [ RFC-322 ]
            Parejkoj John Parejko made changes -
            Link This issue is triggering DM-10153 [ DM-10153 ]
            Hide
            Parejkoj John Parejko added a comment -

            Big review here: thanks for taking it on. If you don't think you can get to it within a week or so, please find someone else to assign it to (I'll likely be in only marginal contact until April 16th).

            Particular things to note:

            • testPhotoCalib.py fails the memory test, triggered by testPersistence (you can show this by {{unittest.skip()}}ing it. I couldn't figure out why. Ideas welcome.
            • Two parts are not implemented, but refer to the implementation tickets. I don't need either of those to start on the jointcal work, so I'm leaving them for later: this has taken enough already. See DM-10153.
            • I had to make several changes to BoundedField to make this work: those are in separate commits. I didn't file tickets about those changes, but maybe I should have, to capture the story points?
            • Please check my math on the error calculations. I think I got it right (thanks Mathematica!), but extra eyes are welcome there.
            • The API is not exactly as described in RFC-289, but the changes were necessary to make it work.
            Show
            Parejkoj John Parejko added a comment - Big review here: thanks for taking it on. If you don't think you can get to it within a week or so, please find someone else to assign it to (I'll likely be in only marginal contact until April 16th). Particular things to note: testPhotoCalib.py fails the memory test, triggered by testPersistence (you can show this by {{unittest.skip()}}ing it. I couldn't figure out why. Ideas welcome. Two parts are not implemented, but refer to the implementation tickets. I don't need either of those to start on the jointcal work, so I'm leaving them for later: this has taken enough already. See DM-10153 . I had to make several changes to BoundedField to make this work: those are in separate commits. I didn't file tickets about those changes, but maybe I should have, to capture the story points? Please check my math on the error calculations. I think I got it right (thanks Mathematica!), but extra eyes are welcome there. The API is not exactly as described in RFC-289 , but the changes were necessary to make it work.
            Parejkoj John Parejko made changes -
            Reviewers John Swinbank [ swinbank ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            swinbank John Swinbank added a comment -

            testPhotoCalib.py fails the memory test, triggered by testPersistence (you can show this by unittest.skip()​ing it. I couldn't figure out why. Ideas welcome.

            I actually see two different memory test failures: one that applies throughout testPhotoCalib.py and one that applies only to testPersistence. I think the following should fix both:

            diff --git a/src/image/PhotoCalib.cc b/src/image/PhotoCalib.cc
            index 2ba998f..d5f48b6 100644
            --- a/src/image/PhotoCalib.cc
            +++ b/src/image/PhotoCalib.cc
            @@ -298,6 +298,7 @@ public:
                     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");
            +        schema.getCitizen().markPersistent();
                 }
             
                 PersistenceHelper(table::Schema const & s) :
            diff --git a/tests/testPhotoCalib.py b/tests/testPhotoCalib.py
            index f9fc68a..a8c5d1c 100644
            --- a/tests/testPhotoCalib.py
            +++ b/tests/testPhotoCalib.py
            @@ -92,6 +92,11 @@ class PhotoCalibTestCase(lsst.utils.tests.TestCase):
                     self.linearXZeroPoint = lsst.afw.math.ChebyshevBoundedField(self.bbox,
                                                                                 np.array([[self.counts0, self.counts0]]))
             
            +    def tearDown(self):
            +        del self.schema
            +        del self.table
            +        del self.catalog
            +
                 def _testPhotoCalibCenter(self, photoCalib, counts0Sigma):
                     """
                     Test conversions of counts for the mean and (0,0) value of a photoCalib.
            

            That said, I note that PersistenceHelper(int tableVersion=PHOTOCALIB_TABLE_CURRENT_VERSION) is public. I'm not sure that you always want to mark the schema as persistent if it's not being called through PersistenceHelper::get(). Mind you, I'm not sure if you ever want to access it except for through PersistenceHelper::get(), so maybe it could just be private?

            Show
            swinbank John Swinbank added a comment - testPhotoCalib.py fails the memory test, triggered by testPersistence (you can show this by unittest.skip() ​ing it. I couldn't figure out why. Ideas welcome. I actually see two different memory test failures: one that applies throughout testPhotoCalib.py and one that applies only to testPersistence . I think the following should fix both: diff --git a/src/image/PhotoCalib.cc b/src/image/PhotoCalib.cc index 2ba998f..d5f48b6 100644 --- a/src/image/PhotoCalib.cc +++ b/src/image/PhotoCalib.cc @@ -298,6 +298,7 @@ public: 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"); + schema.getCitizen().markPersistent(); }   PersistenceHelper(table::Schema const & s) : diff --git a/tests/testPhotoCalib.py b/tests/testPhotoCalib.py index f9fc68a..a8c5d1c 100644 --- a/tests/testPhotoCalib.py +++ b/tests/testPhotoCalib.py @@ -92,6 +92,11 @@ class PhotoCalibTestCase(lsst.utils.tests.TestCase): self.linearXZeroPoint = lsst.afw.math.ChebyshevBoundedField(self.bbox, np.array([[self.counts0, self.counts0]]))   + def tearDown(self): + del self.schema + del self.table + del self.catalog + def _testPhotoCalibCenter(self, photoCalib, counts0Sigma): """ Test conversions of counts for the mean and (0,0) value of a photoCalib. That said, I note that PersistenceHelper(int tableVersion=PHOTOCALIB_TABLE_CURRENT_VERSION) is public. I'm not sure that you always want to mark the schema as persistent if it's not being called through PersistenceHelper::get() . Mind you, I'm not sure if you ever want to access it except for through PersistenceHelper::get() , so maybe it could just be private?
            Hide
            swinbank John Swinbank added a comment - - edited

            Overall: very nice! The implementation seems clear and to the point, and (even better) the test suite is great.

            I have left a number of, mostly fairly minor, comments on GitHub. I was a bit bothered by the large number of quite similar versions of countsToMagnitude/countsToMaggies. I see you've already commented that they are all in active use or have a use coming up, but still — I wondered (but have not checked) if it'd be possible to refactor the callers slightly to avoid pushing all the possible alternatives into this class.

            Also worth noting is that I've no real experience with pybind11. I looked through your code and it seems fine, but it might be worth asking one of the experts (Russell, Krzysztof, Pim, ...) to glance through it in case I'm missing some subtleties.

            Specific points you raise above:

            testPhotoCalib.py fails the memory test, triggered by testPersistence (you can show this by unittest.skip()​ing it. I couldn't figure out why. Ideas welcome.

            I think my comment above should cover this.

            I had to make several changes to BoundedField to make this work: those are in separate commits. I didn't file tickets about those changes, but maybe I should have, to capture the story points?

            I have no strong feeling on this — I'd say they're fine as they are. Maybe Simon Krughoff cares?

            Please check my math on the error calculations. I think I got it right (thanks Mathematica!), but extra eyes are welcome there.

            I tried to work this out on the proverbial envelope back, and I got a log(10) in a different place. I may well be confused, but perhaps you could take another look and feel free to ping me if we disagree.

            Show
            swinbank John Swinbank added a comment - - edited Overall: very nice! The implementation seems clear and to the point, and (even better) the test suite is great. I have left a number of, mostly fairly minor, comments on GitHub. I was a bit bothered by the large number of quite similar versions of countsToMagnitude / countsToMaggies . I see you've already commented that they are all in active use or have a use coming up, but still — I wondered (but have not checked) if it'd be possible to refactor the callers slightly to avoid pushing all the possible alternatives into this class. Also worth noting is that I've no real experience with pybind11. I looked through your code and it seems fine, but it might be worth asking one of the experts (Russell, Krzysztof, Pim, ...) to glance through it in case I'm missing some subtleties. Specific points you raise above: testPhotoCalib.py fails the memory test, triggered by testPersistence (you can show this by unittest.skip() ​ing it. I couldn't figure out why. Ideas welcome. I think my comment above should cover this. I had to make several changes to BoundedField to make this work: those are in separate commits. I didn't file tickets about those changes, but maybe I should have, to capture the story points? I have no strong feeling on this — I'd say they're fine as they are. Maybe Simon Krughoff cares? Please check my math on the error calculations. I think I got it right (thanks Mathematica!), but extra eyes are welcome there. I tried to work this out on the proverbial envelope back, and I got a log(10) in a different place. I may well be confused, but perhaps you could take another look and feel free to ping me if we disagree.
            Hide
            krughoff Simon Krughoff added a comment -

            I have no strong feeling on this — I'd say they're fine as they are. Maybe Simon Krughoff cares?

            If it's more than 4, I'd like to see extra stories.

            Show
            krughoff Simon Krughoff added a comment - I have no strong feeling on this — I'd say they're fine as they are. Maybe Simon Krughoff cares? If it's more than 4, I'd like to see extra stories.
            Parejkoj John Parejko made changes -
            Link This issue relates to DM-10260 [ DM-10260 ]
            Hide
            swinbank John Swinbank added a comment -

            Post-response-to-review comments:

            • I'm still not super happy with the persistence stuff. I take your point that we don't have good documentation on how to do things properly, but, to my uneducated eye, something still looks fishy. For example, if I'm reading things correctly, you now always mark the schema created when you construct a PersistenceHelper as persistent. I think that's correct if you construct it by calling PersistenceHelper::get(), but not if you call the constructor directly. I realise this is unlikely to be a problem in practice (the code lives in an anonymous namespace, so the chances of it being called incorrectly are minimal), but it seems ... unfortunate, and I don't really see why it's necessary.
            • I'd also like to better understand the interaction with the PHOTOCALIB_TABLE_CURRENT_VERSION. I think PersistenceHelper::get() ought to take a version number. If I'm wrong, I'd like to understand why. (I should add that I have no particular expertise on this, so I'm quite likely wrong).
            • I appreciate the extensive discussion on the ticket re the overloaded countsToM... functions. They don't sit entirely easily with me, but I am happy that you've thought through the issues; I'm not requesting any changes here.
            • There are some review comments which you've indicated agreement with, but don't seem to have acted on yet (e.g. BoundedField::operator<<). I assume they're just still in progress.
            Show
            swinbank John Swinbank added a comment - Post-response-to-review comments: I'm still not super happy with the persistence stuff. I take your point that we don't have good documentation on how to do things properly, but, to my uneducated eye, something still looks fishy. For example, if I'm reading things correctly, you now always mark the schema created when you construct a PersistenceHelper as persistent. I think that's correct if you construct it by calling PersistenceHelper::get() , but not if you call the constructor directly. I realise this is unlikely to be a problem in practice (the code lives in an anonymous namespace, so the chances of it being called incorrectly are minimal), but it seems ... unfortunate, and I don't really see why it's necessary. I'd also like to better understand the interaction with the PHOTOCALIB_TABLE_CURRENT_VERSION . I think PersistenceHelper::get() ought to take a version number. If I'm wrong, I'd like to understand why. (I should add that I have no particular expertise on this, so I'm quite likely wrong). I appreciate the extensive discussion on the ticket re the overloaded countsToM... functions. They don't sit entirely easily with me, but I am happy that you've thought through the issues; I'm not requesting any changes here. There are some review comments which you've indicated agreement with, but don't seem to have acted on yet (e.g. BoundedField::operator<< ). I assume they're just still in progress.
            Hide
            swinbank John Swinbank added a comment -

            Additional comments after OOB discussion:

            • I am slightly in favour of throwing when we request a pass a point that falls outside the domain of the BoundedField, but only slightly: I urge you to consider this, but implementing it is not a condition of the review.
            • As you pointed out, it's possible to define a constant PhotoCalib with an empty bbox. What semantics are attached to that? It seems like it could either be read as "valid everywhere" or "valid nowhere". It'd be nice to clarify in the documentation what the intention is (and then to throw or not throw as appropriate, if you act on the first point above).
            Show
            swinbank John Swinbank added a comment - Additional comments after OOB discussion: I am slightly in favour of throwing when we request a pass a point that falls outside the domain of the BoundedField , but only slightly: I urge you to consider this, but implementing it is not a condition of the review. As you pointed out, it's possible to define a constant PhotoCalib with an empty bbox. What semantics are attached to that? It seems like it could either be read as "valid everywhere" or "valid nowhere". It'd be nice to clarify in the documentation what the intention is (and then to throw or not throw as appropriate, if you act on the first point above).
            krzys Krzysztof Findeisen made changes -
            Link This issue has to be done before DM-4639 [ DM-4639 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue has to be done before DM-4639 [ DM-4639 ]
            Hide
            Parejkoj John Parejko added a comment -

            Jim Bosch or John Swinbank : would either of you be able to do a very quick review of the below PR? After I added operator== and toString() to the BoundedField API, I discovered I had to add them to CoaddBoundedField in meas_algorithms. It's +72 lines total.

            https://github.com/lsst/meas_algorithms/pull/77/files

            Given the lack of docstrings in CoaddBoundedField, I did my best.

            Show
            Parejkoj John Parejko added a comment - Jim Bosch or John Swinbank : would either of you be able to do a very quick review of the below PR? After I added operator== and toString() to the BoundedField API, I discovered I had to add them to CoaddBoundedField in meas_algorithms. It's +72 lines total. https://github.com/lsst/meas_algorithms/pull/77/files Given the lack of docstrings in CoaddBoundedField, I did my best.
            Hide
            Parejkoj John Parejko added a comment -

            John Swinbank in the end, I did not implement testing against the bbox when evaluating the BoundedField, but might add such tests as part of DM-10154. I did clarify in the docs what an empty bbox meant ("valid everywhere").

            Show
            Parejkoj John Parejko added a comment - John Swinbank in the end, I did not implement testing against the bbox when evaluating the BoundedField, but might add such tests as part of DM-10154 . I did clarify in the docs what an empty bbox meant ("valid everywhere").
            Hide
            swinbank John Swinbank added a comment -

            Thanks John Parejko!

            I have quickly glanced at your changes in meas_algorithms, and didn't see anything that looked immediately wrong except for this commented throw. I'm not really familiar with the code though, and it's late and my brain is fried. If Jim has chance to look at it that's great, otherwise I'll go over it more carefully tomorrow.

            Other than that, the only thing I wonder about is persistence. Did you have any further changes you wanted to make there? If not, I'll eyeball it again and then I think you're good to go.

            Show
            swinbank John Swinbank added a comment - Thanks John Parejko ! I have quickly glanced at your changes in meas_algorithms, and didn't see anything that looked immediately wrong except for this commented throw . I'm not really familiar with the code though, and it's late and my brain is fried. If Jim has chance to look at it that's great, otherwise I'll go over it more carefully tomorrow. Other than that, the only thing I wonder about is persistence. Did you have any further changes you wanted to make there? If not, I'll eyeball it again and then I think you're good to go.
            Hide
            Parejkoj John Parejko added a comment -

            Whoops on that commented throw! Thanks for catching that. Fixed.

            I believe I already incorporated your persistence suggestions (and the remaining comments from the review). I've already rebase-flattened things, but feel free to take a look.

            Show
            Parejkoj John Parejko added a comment - Whoops on that commented throw! Thanks for catching that. Fixed. I believe I already incorporated your persistence suggestions (and the remaining comments from the review). I've already rebase-flattened things, but feel free to take a look.
            Hide
            jbosch Jim Bosch added a comment -

            One comment on the meas_algorithms PR. Otherwise looks good to me.

            Show
            jbosch Jim Bosch added a comment - One comment on the meas_algorithms PR. Otherwise looks good to me.
            Hide
            swinbank John Swinbank added a comment -

            Looks great. Nice job, John Parejko!

            Show
            swinbank John Swinbank added a comment - Looks great. Nice job, John Parejko !
            swinbank John Swinbank made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for all the help on the review. Jenkins passed (except OSX which is running forever):

            https://ci.lsst.codes/job/stack-os-matrix/23422/

            Merged and done.

            Show
            Parejkoj John Parejko added a comment - Thanks for all the help on the review. Jenkins passed (except OSX which is running forever): https://ci.lsst.codes/job/stack-os-matrix/23422/ Merged and done.
            Parejkoj John Parejko made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-10395 [ DM-10395 ]

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                John Swinbank
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Krzysztof Findeisen, Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel