Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update source to use new Footprint API #19

Merged
merged 1 commit into from Apr 26, 2017
Merged

Update source to use new Footprint API #19

merged 1 commit into from Apr 26, 2017

Conversation

natelust
Copy link
Contributor

No description provided.

Copy link
Contributor

@fred3m fred3m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments. mostly related to comments in other pull requests for this ticket.

@@ -265,7 +257,8 @@ PTR(KronAperture) KronAperture::determineRadius(
//
// Build an elliptical Footprint of the proper size
//
afw::detection::Footprint foot(afw::geom::ellipses::Ellipse(axes, center));
afw::detection::Footprint foot(afw::geom::SpanSet::fromShape(
afw::geom::ellipses::Ellipse(axes, center)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make my comments from previous pull requests more general, I think that one should be able to initialize a Footprint with any object that can be used to initialize a SpanSet. There doesn't appear to be a good reason to me to break the old (and more convenient) API, especially with the new 80 char limit for python code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note here, we have discussed this a bit in person, but a note here for posterity. The API for footprint was chosen to only take a shared pointer to a SpanSet such that 1. Code duplication was minimized (maintaining two similar parallel apis) 2. To encourage developers to used a SpanSet instead of a footprint whenever possible. These two things were weighted against verbosity, and ultimately we decided against verbosity of code

@@ -282,7 +275,8 @@ PTR(KronAperture) KronAperture::determineRadius(
);

try {
iRFunctor.apply(foot);
foot.getSpans()->applyFunctor(
iRFunctor, *(subImage.getImage()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could an applyTo method be added to FootprintFindMoment to mimick the old functionality? This feels unnecessarily clunky.

@natelust natelust merged commit f2b2d43 into master Apr 26, 2017
@ktlim ktlim deleted the tickets/DM-8108 branch August 25, 2018 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants