Tickets/dm 8108 #32

natelust · 2017-04-13T02:04:02Z

No description provided.

fred3m

As a whole I like the changes to the API a lot. I think that the right amount of functionality has been moved to SpanSets to make them very useful objects, and I can think of a lot of places in my code that things will be greatly simplified. Great job Nate!

The main comments I have on this PR are regarding the way certain SpanSet methods return a copy of the original SpanSet as opposed to updating the object in place. This could result in unexpected behavior if a variable s is set to Footprint.spans while sometime later Footprint.spans is updated by a union or clippedTo method, which results in s no longer pointing to Footprint.spans. This is the major disadvantage to making spans a property of Footprint as opposed to having Footprint inherit from SpanSet and adding a few new methods.

I'm also a little curious as to why you changed all of the pointers (PTR(det::Span)) to geom::Span.

fred3m · 2017-04-14T18:35:16Z

python/lsst/meas/deblender/deblend.py

+            spans = src.getFootprint().spans
+            for child in kids:
+                spans = spans.union(child.getFootprint().spans)
+            src.getFootprint().setSpans(spans)


I like the old behavior better in this case, so I'm curious why it was changed. It seems more useful to have SpanSets.union update the SpanSet (spans in this case) in place with (potentially) a list of SpanSets, eliminating the need to execute src.getFootprint().setSpans(spans). Then an afwDetection.union function to take the union of multiple SpanSets without modifying any of them.

fred3m · 2017-04-14T19:03:06Z

python/lsst/meas/deblender/plugins.py

+    foot.setSpans(afwGeom.SpanSet(newSpans))
+    foot.removeOrphanPeaks()
+
+


I'm surprised that clipTo and clipToNonzero are not included in the SpanSet API. Removing empty spans seems like an important feature that is useful outside the deblender, even if it has only been used in this one place in the stack so far.

It's also curious that a working C++ function has been replaced by one in python that will run much slower, especially considering how many Spans will be iterated over to deblend an entire image. Even if there is a good reason to keep this functionality out of afw, I still think this should live in deblendUtils.cc.

Clipped to is a method on a SpanSet. I will evaluate if clipToNonzero should be moved to a different location, which may or may not happen as part of this ticket. I am not sure how I would feel about this being a method on a SpanSet as it really also only makes sense in the context of a specific image and not something geometric, but I could be persuaded one way or the other.
In relation to your speed question, I tweaked the routine to be even faster than the old c++ equivalent.

fred3m · 2017-04-14T19:40:14Z

python/lsst/meas/deblender/plugins.py

    padim = maskedImage.Factory(tbb)
-    afwDet.copyWithinFootprintMaskedImage(fpcopy, maskedImage, padim)
+    fpcopy.spans.clippedTo(maskedImage.getBBox()).copyMaskedImage(maskedImage, padim)



As a whole I like the API changes in this block, including the implementation of dilate and removeOrphanPeaks. But it seems like SpanSet.clippedTo should modify the SpanSet in place as opposed to requiring Footprint.spans to point to a new object.

fred3m · 2017-04-14T19:50:08Z

src/BaselineUtils.cc

-    return (*sp1 < *sp2);
+static bool span_compare(geom::Span const & sp1,
+                         geom::Span const & sp2) {
+    return (sp1 < sp2);


Is there a reason this was changed from a pointer? I'm just thinking of very large blends where a span could be a few hundred pixels across. There doesn't seem to be a reason to copy the spans as opposed to pointing to them.

There is no copy being done, it is simply being passed by reference now instead of my pointer. Same effect with less *

fred3m · 2017-04-14T20:21:38Z

src/BaselineUtils.cc

+            foot->getSpans()->setImage(*dist, static_cast<dtype>(1));
+            ++i;
+        }
+        }


Would it be better to use an ordinary for loop as opposed to a range based for loop, since you still need i?

I don't know in what sense you mean better. From a code efficiency standpoint, it will almost certainly compile out to the same thing. If you mean from a readability standpoint, maybe? If it were a standard for loop it would just move the lines with i in them into the line that declares the loop, and you would then either need to have an iterator object to deference, or use the [] syntax with the vector all over the place. I don't think there is a clear winner, and I preferred how this looked, rather than having a complicated for line. It would be nice if c++ supported enumerations in the range based for loop. Unfortunately we will need to wait until c++ 17 for that I think.

fred3m · 2017-04-14T20:47:23Z

src/BaselineUtils.cc

+        if (strayfoot[i]) {
+            strayfoot[i]->setSpans(std::make_shared<geom::SpanSet>(straySpans[i]));
+        }
+    }


I think this can be moved into the next for loop.

good catch. I was too focused on fixing the above loop, I didn't look at what was below

fred3m · 2017-04-14T20:53:02Z

src/BaselineUtils.cc

-    const SpanList spans = foot.getSpans();
-    assert(foot.isNormalized());
+    auto sfoot = std::make_shared<det::Footprint>();
+    sfoot->setPeakSchema(foot.getPeaks().getSchema());


This seems to come up a lot. Why can't the Footprint be initialized with a Schema anymore?

It can, but only if it is also initialized with a SpanSet at the same time. Which I could have done with an empty SpanSet, I am not sure why I didn't, probably because this seemed more readable [make an empty Footprint, set the schema]. The desire was to keep down the number of constructors, but one that only takes a schema may be worth looking into, I will make a note.

I will note that in general this probably will not be an issue in the future as you would do things differently. I was trying to shoehorn the new code in as exactly as I could into the old pattern

fred3m · 2017-04-14T21:10:32Z

src/BaselineUtils.cc

+                tmpSpans.push_back(geom::Span(fy, cx + dxlo, cx + dxhi));
+            } else {
+                tmpSpans.push_back(geom::Span(fy, cx + dxhi, cx + dxlo));
+            }


I must be missing something, why is this extra if..else block needed? Line 1129 checks if dxlo<=dxhi, so why would cx+dxlo<=cx+dxhi give a different result?

The old code had: sfoot->addSpan(fy, cx + dxlo, cx + dxhi); in it. addSpans had a check on which was lower and would swap the arguments if they were entered wrong (essentially the same if statements moved here) I was trying to ensure that the behavior of addSpans stayed exactly the same. cx is a signed int, so theoretically it could be negative (a wierd xy0 for the image). This could cause what is lower and higher to flip (though now that I think about it longer, only the the case for cx -dx(lo/hi)). This check could probably be removed for the addition, but I would want someone to verify thatI am right in that, as it would be removing a check we used to have.

fred3m · 2017-04-14T21:11:38Z

src/BaselineUtils.cc

+                tmpSpans.push_back(geom::Span(by, cx - dxlo, cx - dxhi));
+            } else {
+                tmpSpans.push_back(geom::Span(by, cx - dxhi, cx - dxlo));
+            }


See my previous comment for why this if..else block also seems unnecessary to me.

fred3m · 2017-04-14T21:18:28Z

src/BaselineUtils.cc

+    std::shared_ptr<geom::SpanSet> edgeSpans = sfoot->getSpans()->findEdgePixels();
+    std::vector<geom::Span> tmpSpans;
+    for (geom::SpanSet::const_iterator ss = edgeSpans->begin();
+         ss != edgeSpans->end(); ++ss) {


Is this the indentation that clang-format created? It seems a bit hard to read to me.

no, I don't know what happened there, it didn't even need to be on a different line, changed now

fred3m requested changes Apr 14, 2017

View reviewed changes

natelust added 2 commits April 24, 2017 13:27

Update deblender code to use new Footprint API

272014c

Update deblender unittest for new Footprint APIs

e9c9c7e

natelust force-pushed the tickets/DM-8108 branch from 762f731 to e9c9c7e Compare April 24, 2017 18:28

natelust merged commit e9c9c7e into master Apr 26, 2017

ktlim deleted the tickets/DM-8108 branch August 25, 2018 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tickets/dm 8108 #32

Tickets/dm 8108 #32

natelust commented Apr 13, 2017

fred3m left a comment

fred3m Apr 14, 2017

fred3m Apr 14, 2017

natelust Apr 24, 2017

fred3m Apr 14, 2017

fred3m Apr 14, 2017

natelust Apr 20, 2017

fred3m Apr 14, 2017

natelust Apr 21, 2017

fred3m Apr 14, 2017

natelust Apr 21, 2017

fred3m Apr 14, 2017

natelust Apr 21, 2017

natelust Apr 21, 2017

fred3m Apr 14, 2017

natelust Apr 21, 2017

fred3m Apr 14, 2017

fred3m Apr 14, 2017

natelust Apr 21, 2017

		foot.setSpans(afwGeom.SpanSet(newSpans))
		foot.removeOrphanPeaks()

Tickets/dm 8108 #32

Tickets/dm 8108 #32

Conversation

natelust commented Apr 13, 2017

fred3m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment