Fix Version/s: None
Component/s: afw, meas_algorithms
Sprint:Science Pipelines DM-S15-5
Team:Data Release Production
|Field||Original Value||New Value|
|Reviewers||Russell Owen [ rowen ]|
|Status||To Do [ 10001 ]||In Review [ 10004 ]|
Review of afw changes:
Overall this looks great.
One request: in the descriptions of the various modified Exposure methods please document that includeValidPolygon is silently ignored if the Exposure has no valid polygon. One could imagine other ways of handling this, so it's best to be explicit.
At risk of being too picky, please use curly braces in all control structures. Our coding standard do call for that (see 6-7 in https://confluence.lsstcorp.org/pages/viewpage.action?pageId=16908737#C++LayoutandComments-Layout) and I find the consistency really helps readability.
Please file a ticket to add a unit test for this new functionality and link it to this ticket.
Review of meas_algorithms changes
Another nice clean change, and the addition of a unit test is much appreciated!
This is mostly in the category of cleaning up existing documentation, and thus possibly out of scope, but...
- It would helpful if getValidPolygon was documented to throw pex::exceptions::RangeError if index was out of range, and likewise for all the other get methods that take an index.
- It would be helpful if the doc strings were only in one place – preferably the .h file. Having them in two places can lead to one going stale (and not necessarily the one that will be picked up by Doxygen or read by users).
|Status||Reviewed [ 10101 ]||In Review [ 10004 ]|
Thanks! Ticket for the missing tests is on
DM-3349. Will merge to master shortly.
|Resolution||Done [ 10000 ]|
|Status||Reviewed [ 10101 ]||Done [ 10002 ]|
Russell, would you mind taking a look? The work is on:
It's mostly a straightforward cherry pick from HSC, but I did some tidying up to better comply with LSST standards.