Review for DM-2306: Transforms for Centroid Measurements #14
Conversation
We will use the same scaffolding with slighty different methods for setting and checking fields for testing other transform types.
Thereby make the test code robust against changes to the underlying delimeter.
| public: | ||
| typedef GaussianCentroidControl Control; | ||
| GaussianCentroidTransform(Control const & ctrl, std::string const & name, afw::table::SchemaMapper & mapper); | ||
| }; |
There was a problem hiding this comment.
It's much more common in our codebase to have blank lines between each method, typedef, and member in a class (see standard 6-19, though reading that just now is the first time I've noticed that we actually suggest two blank lines in source files).
|
This looks really good - I especially like the Aside from the really nitpicky things above, my only big-picture question is whether we need the |
|
Thanks for the review! Regarding your 'big picture' comment, I guess this depends on the likely longer-term uses of the code. It seemed not implausible to me that sometime in the future folks would want to unpack celestial positions from tables into code, and this makes it easy for them to do so. You probably have a better idea than I do as to whether that's long-term-useful, though, and I certainly agree that it's not being used anywhere today, so I have no objection to rearranging things as you suggest. |
|
I suspect anyone wanting extract these values from tables would also want to extract the flags as well, and hence aggregating 2 of the 3 sets of fields they'd want doesn't really gain them much (and unfortunately, extracting the flags in a generic-but-safe way is much harder). |
jdswinbank
force-pushed
the
tickets/DM-2306
branch
from
6a5c517
to
36fb890
Compare
Make it possible to use the same transformation test framework for both single frame and forced measurement plugins, which require slightly different initialization.
Previously, this was either not checked or checked only via assertion (which may be disabled or result in premature termination). By throw an appropriate exception, the error can be caught and handled in the calling code.
Provide a specialized version of TransformTestCase that populates the input of, and checks the output of, centroid transformations.
Remove unused include statements, or convert them to import where necessary, thereby preventing duplicate exports of C++ code. As well as being tidier, this is necessary to make the behaviour of SillyCentroid consistent with the rest of the stack. For example, raising an exception in SillyCentroid would previously raise `testLib.ExceptionName` rather than (e.g.) `measBase.ExceptionName`; it would not therefore be caught by our standard exception handling mechanisms.
These algorithms should report errors, but currently don't so there's no point allocating space for them. See DM-2368 & -2369.
The PeakCentroid algorithm is implemented in Python, therefore it requires a Python-based transform. Separate measurement implementations are used for single frame and forced algorithms, but only a single transform is required.
jdswinbank
force-pushed
the
tickets/DM-2306
branch
from
36fb890
to
39d2b46
Compare
|
@TallJimbo -- branch updated to take account of your suggestions. Currently waiting for buildbot; once that runs successfully, I'll merge to master unless you indicate you'd like to take a further look. |
No description provided.