# Implement new spatially-variable PhotoCalib model

XMLWordPrintable

## Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
16
• Sprint:
• Team:

## 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.

## Activity

Hide
John Parejko added a comment -

Beginning to implement the design described in RFC-289.

Show
John Parejko added a comment - Beginning to implement the design described in RFC-289 .
Hide
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
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.
Hide
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("countsMag0Sigma", "1-sigma error on countsmag0", "count");  isConstant = schema.addField("isConstant", "Is this spatially-constant?");  field = schema.addField("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
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
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
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
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
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.
Hide
John Swinbank added a comment -

• 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
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
John Swinbank added a comment -

• 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
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).
Hide
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
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
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
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
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
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
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
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
Jim Bosch added a comment -

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

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

Looks great. Nice job, John Parejko!

Show
John Swinbank added a comment - Looks great. Nice job, John Parejko !
Hide
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
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.

## People

• Assignee:
John Parejko
Reporter:
John Parejko
Reviewers:
John Swinbank
Watchers:
Jim Bosch, John Parejko, John Swinbank, Krzysztof Findeisen, Russell Owen, Simon Krughoff