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

Tickets/dm 9828 #72

Merged
merged 3 commits into from Mar 31, 2017
Merged

Tickets/dm 9828 #72

merged 3 commits into from Mar 31, 2017

Conversation

yalsayyad
Copy link
Contributor

No description provided.

Copy link
Contributor

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

This looks great!
I love that you cleaned up the order stuff, but maybe that should be on a separate commit.
Can you explain in the commit message why Akima should be the default?

@param[in] algorithm name of interpolation algorithm; if None use self.config.algorithm

@return fit background as an lsst.afw.math.Background

@throw RuntimeError if lsst.afw.math.makeBackground returns None,
which is apparently one way it indicates failure
"""

binSizeX = self.config.binSizeX if self.config.binSizeX else self.config.binSize
binSizeY = self.config.binSizeY if self.config.binSizeY else self.config.binSize
Copy link
Contributor

Choose a reason for hiding this comment

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

Add explicit == 0? Makes it clearer and corresponds more exactly with your config docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

elif self.config.undersampleStyle == "REDUCE_INTERP_ORDER":
if order < 1:
raise ValueError("Cannot reduce approxOrder below 0. " +
"Try using undersampleStyle = \"INCREASE_NXNYSAMPLE\" instead?")
order = min(nx, ny) - 1
self.log.warn("Reducing approxOrder to %d" % order)
elif self.config.undersampleStyle == "INCREASE_NXNYSAMPLE":
# Reduce bin size to the largest acceptable square bins
Copy link
Contributor

Choose a reason for hiding this comment

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

Do they have to be square? The order is (unfortunately) identical for both dimensions, but I don't think there's a reason the bin size has to be identical.

Copy link
Contributor Author

@yalsayyad yalsayyad Mar 30, 2017

Choose a reason for hiding this comment

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

They don't. A goal with this ticket was not to change behavior of existing configs. As I mentioned on the ticket comments, its not obvious what behavior is more intuitive and expected here: to reduce the bin size only in the dimension with too few data points or reduce the bin size in both dimensions. Given that everyone up until this point has specified square bins, I thought it would be least surprising if the bins were reduced in both dimensions as they were before this ticket.

Add config parameters binSizeX and binSizeY which control
the bin size in the x and y dimensions individually.
If either is 0, the default, the general binSize parameter
is used for the bin size in that dimension.
Applies to both polynomial and spline background models.
of order and config.approxOrderX in the undersampling check in
SubtractBackgroundTask. This change does not alter
behavior because the order is assigned config.approxOrderX.
Chebyshev polynomials are the default background model,
but when splines are requested, the default interpolation
style should be the more stable and robust AKIMA_SPLINE,
to be consistent with other background subtraction classes
and documentation.
@yalsayyad yalsayyad merged commit c795208 into master Mar 31, 2017
@ktlim ktlim deleted the tickets/DM-9828 branch August 25, 2018 06:50
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