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

Add general Background classes #370

Merged
merged 22 commits into from
Jun 20, 2016
Merged

Conversation

larrybradley
Copy link
Member

@larrybradley larrybradley commented Jun 10, 2016

This large PR contains the following changes:

  1. Turns the background.py module into a background subpackage.
  2. Adds many new classes for computing a (scalar) background value or background rms value from a (possibly masked) array of any dimension:
  • MeanBackground
  • MedianBackground
  • MMMBackground
  • SExtractorBackground
  • BiweightLocationBackground
  • StdBackgroundRMS
  • MADStdBackgroundRMS
  • BiweightMidvarianceBackgroundRMS

Sigma-clipping is performed by default, but can be turned off by setting sigclip = False.
3. The existing BackgroundBase, Background and BackgroundIDW classes are now called BackgroundBase2D, Background2D, and BackgroundIDW2D, respectively, since they return 2D background images.

My plan is to submit another PR to integrate the general background classes into the 2D background classes (with a similar interface).

Example

from photutils import MMMBackground
bkg = MMMBackground(sigma=3., iters=10)
bkg_value = bkg.calc_background(data)

The classes are designed to be used as an object-oriented interface for other functions, e.g.:

bkg = MMMBackground(sigma_lower=5.5, sigma_upper=3., iters=10)
result = psf_photometry(data, ... , background_sub=bkg)

in function form (requires calc_background -> __call__):

result = psf_photometry(data, ... , background_sub=np.ma.mean)

where the user can define custom background classes if desired.

cc: @eteq, @hcferguson

@larrybradley larrybradley added this to the 0.3 milestone Jun 10, 2016
@larrybradley larrybradley self-assigned this Jun 10, 2016
@larrybradley larrybradley force-pushed the background branch 8 times, most recently from 592bd3b to b344016 Compare June 14, 2016 03:53
@larrybradley larrybradley mentioned this pull request Jun 15, 2016
@larrybradley
Copy link
Member Author

The background classes are now callable. The __call__ method simply passes the data to the calc_background() method.

@@ -158,13 +158,13 @@ mesh of a grid that covers the input data to create a low-resolution
background map. The final background or background rms map can then
be generated by interpolating the low-resolution map.
Copy link
Member

Choose a reason for hiding this comment

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

I guess this comment really belongs just a bit further up... But I wonder if you should add a short example in the previous section that demos all of the various Background (not 2D) classes? Right now it all uses the sigma clipping functions, but now you can insert all of the various estimators implemented here. Maybe this could just be a (logarithmic) histogram of the pixel values and then mark the output of the estimators as vertical lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I didn't add the new Background classes to narrative docs yet. I wanted to wait until the API was firmly decided.

@eteq
Copy link
Member

eteq commented Jun 16, 2016

@larrybradley - left some inline comments above, and I've got one more general suggestion:
I found the base hierarchy at first conceptually confusing - that is, why is there a BkgBase at all, when it didn't seem to do anything related to background. After some thought, I realized it's there because it provides sigma-clipping... But I think that doesn't actually need to be the base of the background heirarchy... And in fact it's a bit confusing because it implies background subtraction must involve sigma clipping.

So I have an alternate suggestion that I think is just a small perturbation on what you've done already: change BkgBase to something like SigmaClipper, and use it as a mixin, rather than the base. That is, have BackgroundBase and BackgroundRMSBase be object subclasses, but have all the other classes have inheritance signatures along the lines of MMMBackground(BackgroundBase, SigmaClipper). You might have to be a bit more careful with the super calls, but I think it's a non-issue here because there's no BackgroundBase.__init__ anyway.

@larrybradley
Copy link
Member Author

Thanks, @eteq! Please review the updates.

@eteq
Copy link
Member

eteq commented Jun 19, 2016

looks great to me now, @larrybradley ... except for the test failures. Any idea what might be specific about numpy 1.9 that would cause this?

@larrybradley
Copy link
Member Author

@eteq Thanks.

I guess you missed the ongoing saga with conda and ci-helpers [e.g. 1, 2, 3]. The very short summary is that conda 4.1.0 broke a lot of things last week (among other things leaving out the default channel). Our conda channels are in a state of flux. I think the plan was to wipe astropy-ci-extras and start over in conda-channel-ci-extras, but nothing is in there yet. travis-ci tests are failing across all astropy packages, including the core package. conda 4.1.1 and 4.1.2 are out now, but I think we need to re-build astropy with older numpy versions.

[1] astropy/ci-helpers#97
[2] astropy/ci-helpers#98
[3] astropy/ci-helpers#100

@eteq
Copy link
Member

eteq commented Jun 19, 2016

Ohhh, gotcha - I was confused by the fact that all of the other versions still work, but I see now from those ci issues that 1.9 is indeed problematic.

But given that the others passed, I would think this is good to merge (assuming you're done making changes, @larrybradley ?)

@larrybradley larrybradley merged commit 4e206cc into astropy:master Jun 20, 2016
@larrybradley larrybradley deleted the background branch June 20, 2016 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants