SpanSets #106
SpanSets #106
Conversation
| namespace geom = lsst::afw::geom; | ||
|
|
||
| // Expose properties of the underlying vector constaining spans such that the | ||
| // SpanSet can be concidered a container |
There was a problem hiding this comment.
concidered -> "considered"
|
|
||
| class SpanSet { | ||
| public: | ||
| typedef std::vector<Span>::const_iterator const_iterator; |
There was a problem hiding this comment.
Having just reading through the C++ style guidelines, seems that typedefs should have the first letter capitalized. (Guideline 3-5)
There was a problem hiding this comment.
Hmm, but not for STL standard conventions I hope?
There was a problem hiding this comment.
We should use the STL lowercase form here. I believe this exception the our guidelines is broadly recognized, but it may need to be made explicit if it isn't already.
| }; | ||
| } // namespace | ||
|
|
||
| namespace geom = lsst::afw::geom; |
There was a problem hiding this comment.
According to style guideline 3-6, should be afwGeom.
There was a problem hiding this comment.
Actually, we shouldn't need this namespace alias here at all; names in afw::geom are already visible.
| return spanVector.empty(); | ||
| } | ||
|
|
||
| // Default constructor, creates a null null spanset which may be useful for |
| } | ||
|
|
||
| // Default constructor, creates a null null spanset which may be useful for | ||
| // compairisons |
There was a problem hiding this comment.
"compairisons" -> "comparisons"
| /* Because the array is sorted, the minimum and maximum values for Y will | ||
| be in the first and last elements, only need to find the min and max X | ||
| values */ | ||
| int minX = spanVector[0].getMinX(); |
There was a problem hiding this comment.
Do you need to check here if spanVector is empty?
There was a problem hiding this comment.
Good point, it is possible that the constructor will be abused
| for (auto iter = spanVector.begin() + 1 ; iter != spanVector.end() ; iter++) { | ||
| // This means they are on the same row, and are not contiguous in x | ||
| if ((*(iter -1)).getY() == (*iter).getY() && !spansContiguous(*(iter - 1), *iter, false)) { | ||
| // If two spans are not contiguous on the same row, they must bothe be contiguous with either |
| auto yRange = std::equal_range(primaryRuns.begin(), primaryRuns.end(), y, ComparePrimaryRunY()); | ||
|
|
||
| /* Discard runs for any balue of y for wich we find fewer groups than M, | ||
| * the total Y range of the structuring element. THis is step 3.1 of the |
| } | ||
| } | ||
| } | ||
| return geom::SpanSet(std::move(tempVec), false); |
There was a problem hiding this comment.
Do you need to nomalize the SpanSet here in case one span from other overlaps two spans?
| if (spansOverlap(spn, otherSpn)) { | ||
| added = 1; | ||
| /* To handle one span containing the other, the spans will be added | ||
| * peice wise, and let the SpanSet contructor normalize spans which |
There was a problem hiding this comment.
"peice wise" -> "piecewise"
| } | ||
| } | ||
| } | ||
| /* If added is still zero, that means it did not verlap any of the spans in other |
| static SpanSet spanSetFromShape(int r, Stencil s = Stencil::CIRCLE); | ||
|
|
||
| private: | ||
| void runNormalize(); |
There was a problem hiding this comment.
According to the style guideline 3-4, private member functions must be preceded by an underscore.
| #include "ndarray.h" | ||
| #include "Eigen/Core" | ||
|
|
||
| namespace geom = lsst::afw::geom; |
| void testShiftedBy() { | ||
| geom::Extent2I SSextent(geom::Point2I(2, 2)); | ||
| // BBox lower corner should be at -2,-2 | ||
| auto SpanSetNoShift = geom::SpanSet::spanSetFromShape(2); |
There was a problem hiding this comment.
I think it would be more clear to use the explicit enum values so I can tell what shape you are using.
| template <typename Pixel, int inC> | ||
| ndarray::Array<Pixel, 1, 1> flatten(ndarray::Array<Pixel, 2, inC> & input, | ||
| Point2I const xy0 = Point2I()) const { | ||
| // Populate a one dimensional array with the values from input at taken at the points of SpanSet |
There was a problem hiding this comment.
I didn't see any problems with the use of variadic templates as such, but I did find the code difficult to read and understand. Please consider the suggested style/name changes, and please add documentation. I'd like to re-read the code once that's done; maybe I'll spot something I missed in the current version.
Also, please consider making the unit test coverage a bit more complete. See the comments for specific suggestions.
| return outputArray; | ||
| } | ||
|
|
||
| template <typename Pixel, int outC, int inC> |
There was a problem hiding this comment.
Please document all public and protected members of SpanSets, as well as the class itself. You may want to ensure you've covered everything in RFC-225, but even documentation according to existing guidelines would make the code much more usable and maintainable.
In this case, it's not clear what xy0 is -- the dimensions of input?
| Point2I const xy0 = Point2I()) const { | ||
| // Populate array output with values from input at positions given by SpanSet | ||
| applyFunctor([](Point2I point, int n, typename details::FlatNdGetter<Pixel>::Reference out, | ||
| typename details::ImageNdGetter<Pixel, 2, inC>::Reference in) {out = in;}, |
There was a problem hiding this comment.
This is hard to read. Consider factoring out the types into alias declarations, factoring the lambda into its own variable, and/or listing the lambda parameters one per line (examples in the dev guide).
| template <typename Pixel, int outC, int inC> | ||
| void unflatten(ndarray::Array<Pixel, 2, outC> & output, | ||
| ndarray::Array<Pixel, 1, inC> & input, | ||
| Point2I const xy0 = Point2I()) const { |
There was a problem hiding this comment.
Same comments as for flatten. The internal comments could be a good starting point for documentation, although it's not clear to me what "locations defined by SpanSet" means. Is xy0 an offset or a pair of dimensions?
| } | ||
|
|
||
| template <typename T> | ||
| void setMask(lsst::afw::image::Mask<T> & target, T bitmask) const { |
There was a problem hiding this comment.
Same comments as for unflatten.
| } | ||
|
|
||
| template <typename T> | ||
| void clearMask(lsst::afw::image::Mask<T> & target, T bitmask) const { |
There was a problem hiding this comment.
Same comments as for unflatten.
|
|
||
| void testFunctor() { | ||
| // Test the remaining types of functors. Above code has tested ndarray functors | ||
| // need to test, constants, iterators, Images |
There was a problem hiding this comment.
Your test cases should include some functors that make use of the initial Point2I and int arguments -- there's not a single example in this PR.
Test cases where you call applyFunctor from a non-contiguous SpanSet and/or pick functors that take more than four parameters would also expand coverage.
| SSShape.applyFunctor([](geom::Point2I, int n, int & out, int in){out = in;}, vecObject.begin(), 6); | ||
| auto imageIter = imageObject.begin(); | ||
| auto vectorIter = vecObject.begin(); | ||
| for (int i = 0; i < 25; ++i, ++imageIter, ++vectorIter) { |
There was a problem hiding this comment.
Do you really need three independent counter variables? Can't you just use a foreach loop over imageObject and another over vecObject?
There was a problem hiding this comment.
changed algorithm all together
| lsst::afw::image::Image<int> imageObject(5, 5, 0); | ||
| std::vector<int> vecObject(25, 0); | ||
|
|
||
| auto SSShape = geom::SpanSet::spanSetFromShape(2, geom::Stencil::BOX).shiftedBy(2, 2); |
There was a problem hiding this comment.
As I understand it, this SpanSet covers every point in imageObject and vecObject. This means you can't test whether only desired points are altered; this is particularly important for IterGetter, which has no safeguards against that sort of thing.
There was a problem hiding this comment.
included points outside SpanSet region
| void testUnflatten() { | ||
| // Test version without output array, as this simply delegates | ||
| ndarray::Array<int, 1, 1> input = ndarray::allocate(6); | ||
| input.deep() = 9; |
There was a problem hiding this comment.
Magic numbers should be factored out as constants.
There was a problem hiding this comment.
I have addressed many of these where I felt it appropriate
| std::cout << "SS is " << val << std::endl; | ||
| } | ||
| SSShape.applyFunctor([](geom::Point2I pt, int n, afwImage::Image<int>::Pixel & out, int in){ out = in; }, | ||
| imageObject, 6); |
There was a problem hiding this comment.
Again, factor out the 6 to a named constant.
| invocation. All other arguments to the functor are generated from the getters | ||
| and will be supplied in the order in which they are passed to applyFunctor | ||
| */ | ||
| applyFunctorImpl(func, details::makeGetter(x)...); |
natelust
force-pushed
the
tickets/DM-7170
branch
from
d38c48c
to
346bd30
Compare
| // offset by xy0 | ||
| applyFunctor([](Point2I point, int n, typename details::ImageNdGetter<Pixel, 2, outC>::Reference out, | ||
| typename details::FlatNdGetter<Pixel>::Reference in) {out = in;}, | ||
| ndarray::ndImage(output, xy0), ndarray::ndFlat(input)); |
There was a problem hiding this comment.
Could you use more whitespace in multi-line lambdas like this one (there are a few more in this file) to make them easier to read? At the very least, the lambda implementation should not be on the same line as part of the argument list if the argument list consumes more than one line. For example, I'd reformat this as:
applyFunctor(
[](
Point2I point,
int n,
typename details::ImageNdGetter<Pixel, 2, outC>::Reference out,
typename details::FlatNdGetter<Pixel>::Reference in
) {
out = in;
},
ndarray::ndImage(output, xy0),
ndarray::ndFlat(input)
);
(Also, you should pass the Point by const reference here as well).
| bool inSpan = false; // Are we in a span? | ||
| int start = -1; // Start of span | ||
|
|
||
| for (int x = newBBoxI.getBeginX(); x < newBBoxI.getEndY(); ++x) { |
There was a problem hiding this comment.
newBoxI.getEndY() ->newBoxI.getEndX()?
natelust
force-pushed
the
tickets/DM-7170
branch
2 times, most recently
from
c8f8e1e
to
7060e7f
Compare
There was a problem hiding this comment.
It looks much better; while I would like some more improvements in the unit tests and have some stylistic comments elsewhere, the only serious issue is in the const-correctness of makeGetter(lsst::afw::image::Image<T> const &) and makeGetter(lsst::afw::image::Mask<T> const &).
I think if you try to call these methods from the tests, you'll find that the code refuses to compile unless you make the return type ImageNdGetter<T const, 2, 1>. Please investigate and fix if necessary; I have no objections to merging once that's taken care of.
| #ifndef LSST_AFW_GEOM_SPANSET_H | ||
| #define LSST_AFW_GEOM_SPANSET_H | ||
| /** | ||
| * @brief Implements a compact representation of a collection of pixels |
There was a problem hiding this comment.
This block will be attached to either nothing or the lsst namespace; remove it.
| /** @brief An enumeration class which describes the shape used in creating SpanSets, | ||
| erosion kernels, or dilation kernels. CIRCLE creates a circle shape, BOX | ||
| creates a box shape, and MANHATTAN creates a diamond shape. | ||
| */ |
There was a problem hiding this comment.
Brief descriptions should be one line. Documentation of the enum values makes more sense as part of the detailed description (or immediately before each value, of course).
| * | ||
| * @tparam Pixel - The data-type for the ndarray | ||
| * @tparam inC - Number of guaranteed row-major contiguous dimensions, starting from the end | ||
| */ |
There was a problem hiding this comment.
No documentation of outC. If you plan to say the same thing, you can do @tparam outC, inC.
| * | ||
| * @param output - The 1d ndarray which will be populated with output parameters, will happen in place | ||
| * @param input - The ndarray from which the values will be taken | ||
| * @param xy0 - A point object with is used as the origin point for the SpanSet coordinate system |
| * @param xy0 - A point object with is used as the origin point for the SpanSet coordinate system | ||
| * | ||
| * @tparam Pixel - The datatype for the ndarray | ||
| * @tparam inC - Number of guaranteed row-major contiguous dimensions, starting from the end |
There was a problem hiding this comment.
Again, outC is conspicuously absent.
|
|
||
| // Test with an array for an output | ||
| int outputSize = 6; | ||
| ndarray::Array<int, 2, 2> unflatOutput = ndarray::allocate(ndarray::makeVector(outputSize, outputSize)); |
There was a problem hiding this comment.
Trying an array with different outC would give slightly broader test coverage.
| auto spnSt = afwGeom::SpanSet::spanSetFromShape(3, afwGeom::Stencil::CIRCLE)->shiftedBy(5, 5); | ||
| spnSt->setMask(msk, static_cast<lsst::afw::image::MaskPixel>(2)); | ||
| return std::pair<lsst::afw::image::Mask<lsst::afw::image::MaskPixel>, | ||
| std::shared_ptr<afwGeom::SpanSet>>(msk, spnSt); |
There was a problem hiding this comment.
return std::make_pair(msk, spnSt) would be a lot easier to read.
| auto mask = result.first; | ||
| auto spnSt = result.second; | ||
| // Verify the mask was populated with the value 2 | ||
| auto mskArray = mask.getArray(); |
There was a problem hiding this comment.
This test still doesn't check two important properties of setMask:
- does setMask actually set bits, instead of e.g. integer assignment (the degeneracy would be broken if at least some of the original values were non-zero)?
- does setMask only alter points inside the SpanSet, leaving other values unchanged?
| auto mskArray = mask.getArray(); | ||
| for (auto const & spn : *spnSt) { | ||
| for (auto const & pt : spn) { | ||
| BOOST_CHECK(mskArray[pt.getY()][pt.getX()] == static_cast<lsst::afw::image::MaskPixel>(0)); |
There was a problem hiding this comment.
As above, there's no constraint on clearMask actually doing a bitwise operation. While the null case below does check that values are unchanged, it would also be helpful to test this for a nontrivial SpanSet.
| SSShape->applyFunctor([](afwGeom::Point2I, int & out, int in){out = in;}, | ||
| vecObject.begin(), dataValue); | ||
|
|
||
| // Check on the point input of a functor |
There was a problem hiding this comment.
Consider moving this block to after the dataValue/initialValue checks, so that related code is together.
| */ | ||
| std::shared_ptr<SpanSet> union_(SpanSet const & other) const; | ||
|
|
||
| // Compareison Operators |
There was a problem hiding this comment.
"Compareison" -> "Comparison"
| return _bBox; | ||
| } | ||
|
|
||
| /* Here is a description of how the _makeLabels and _label function works. In the |
There was a problem hiding this comment.
"function works" -> "functions work"
| under consideration. This results in all Spans which overlap each other in x | ||
| being labeled with the current label. | ||
|
|
||
| Once _label reaches the end of Spans which meet it's criteria, control falls |
There was a problem hiding this comment.
Is there an extra space after "Spans"?
| back to the _makeLabels function and the current label is incremented. The | ||
| function then moves onto the next Span in the SpanSet which has not been | ||
| labeled. If all spans were labeled in the recursive call the loop falls to the | ||
| end, and the label vector and the last label number (number of labels) are |
There was a problem hiding this comment.
Extra space after "label number"
| void _initialize(); | ||
|
|
||
| /* Label Spans according to contiguous group. If the SpanSet is contiguous, all Spans will be labeled 1. | ||
| * If there are more than one groups each group will receive a label one higher than the previous. |
There was a problem hiding this comment.
"there are more than one groups" -> "there is more than one group"
| } | ||
| ++index; | ||
| } | ||
| return std::pair<std::vector<std::size_t>, std::size_t>(labelVector, currentLabel); |
There was a problem hiding this comment.
Doesn't this return one more than the last label number? That isn't what the comment above says.
natelust
force-pushed
the
tickets/DM-7170
branch
2 times, most recently
from
02d7887
to
af8b530
Compare
SpanSet is a class which holds a collection of spans to represent a collection of spatial indexes. This class has associated methods for working with SpanSets such as intersection, logical difference, and union. It also contains functionality which takes in a functor, and containers to operate on, and applies the functor to the containers at each of the points defined in the span set.
Wrap the new SpanSet class in Swig. Some functionally works, but some will be better wrapped with the upcoming pybind11 change
Added a unit test for SpanSets class in c++ instead of python to more throughly test all of the functionality, specifically with functors, which would be difficult to completely test in python
natelust
force-pushed
the
tickets/DM-7170
branch
from
af8b530
to
6d44d56
Compare
Pull request for simultaneous review of DM-2427, DM-7170, and others. (@rearmstr, can you add to this list?)